Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pispbe/rpi 6.6.y/split jobs handling v2 #6324

Conversation

jmondi
Copy link
Contributor

@jmondi jmondi commented Aug 26, 2024

No description provided.

Jacopo Mondi added 4 commits August 26, 2024 12:24
A comment in the pisp_be driver references to the
 pispbe_schedule_internal() which doesn't exist.

Drop it.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
The config parameters buffer is already validated in
pisp_be_validate_config() at .buf_prepare() time.

However some of the same validations are also performed at
pispbe_schedule() time. In particular the function checks that:

1) config.num_tiles is valid
2) At least one of the BAYER or RGB input is enabled

The input validation is already performed in pisp_be_validate_config()
and there is no need to repeat that at pispbe_schedule() time.

The num_tiles validation can be moved to pisp_be_validate_config() as
well. As num_tiles is a u32 it can'be be < 0, so change the sanity
check accordingly.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Currently the 'pispbe_schedule()' function does two things:

1) Tries to assemble a job by inspecting all the video node queues
   to make sure all the required buffers are available
2) Submit the job to the hardware

The pispbe_schedule() function is called at:

- video device start_streaming() time
- video device qbuf() time
- irq handler

As assembling a job requires inspecting all queues, it is a rather
time consuming operation which is better not run in IRQ context.

To avoid the executing the time consuming job creation in interrupt
context split the job creation and job scheduling in two distinct
operations. When a well-formed job is created, append it to the
newly introduced 'pispbe->job_queue' where it will be dequeued from
by the scheduling routine.

At start_streaming() and qbuf() time immediately try to schedule a job
if one has been created as the irq handler routing is only called when
a job has completed, and we can't solely rely on it for scheduling new
jobs.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
The pisp_be driver uses and depends on runtime_pm.

During the probe() routine, the driver needs to power up the interface
in order to identify and initialize the hardware and it later suspends
it at the end of probe().

The driver erroneously resumes the interface by calling the
pispbe_runtime_resume() function directly, without going
through the pm_runtime helpers, but later suspends it by calling
pm_runtime_put_autosuspend().

This causes a PM usage count imbalance at probe time, notified by the
runtime_pm framework with the below message in the system log:

 pispbe 1000880000.pisp_be: Runtime PM usage count underflow!

Fix this by resuming the interface using the pm runtime helpers instead
of calling the resume function directly.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
@naushir
Copy link
Contributor

naushir commented Aug 30, 2024

All changes look reasonable to me. @jmondi did you test with dual camera running?

@naushir
Copy link
Contributor

naushir commented Aug 30, 2024

PS: Sorry for the delay, but I will respond on the linux-media list shortly as well.

@jmondi
Copy link
Contributor Author

jmondi commented Aug 30, 2024

All changes look reasonable to me. @jmondi did you test with dual camera running?

Yup! Same results as v1, I was puzzled that I got the ov5647 running at 10fps and thought it was something related to this patch, but I get the same results with plain rpi-6.6.y

@naushir
Copy link
Contributor

naushir commented Aug 30, 2024

LGTM. Merging now.

@jmondi
Copy link
Contributor Author

jmondi commented Aug 30, 2024

LGTM. Merging now.

Or should we wait for comments upstream so that we're merging the same version ?

@naushir
Copy link
Contributor

naushir commented Aug 30, 2024

LGTM. Merging now.

Or should we wait for comments upstream so that we're merging the same version ?

Ok sounds sensible.

@jmondi
Copy link
Contributor Author

jmondi commented Sep 4, 2024

closing as I've update the original one in #6302

@jmondi jmondi closed this Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants