media: pisp-be: Split jobs creation and scheduling #6302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| */ | ||
| spin_unlock_irqrestore(&pispbe->hw_lock, flags); | ||
|
|
||
| if (job->config->num_tiles <= 0 || |
There was a problem hiding this comment.
Is the above comment ("We can kick the job off without the hw_lock...") still relevant?
(I must admit I only partially understood it before... but it seems unnecessary now if this function doesn't actually start the job).
There was a problem hiding this comment.
I think it still is relevant, as we're actually kicking off a job without the hw_lock being held, as the driver relies on the hw_busy flag to discern if the system is running a job or not ?
|
It's a moderately involved change so difficult to review, but LGTM. The principle certainly seems to be a good one. |
I was mostly wondering if you have more sophisticated test tools to validate if this change impact in any way on performances. I've run two a concurrent instances of qcam with an ov5647 and and imx219 and things looked ok apart from the fact the ov5647 runs at 7fps compared to the 15 fps at max res the datasheet claims. I'll soon test with your 6.6.y tree to see if it behaves the same. |
|
I can confirm I get the same framerates with |
added 4 commits
September 4, 2024 09:43
A comment in the pisp_be driver references the pispbe_schedule_internal() function which doesn't exist. Drop it. Signed-off-by: Jacopo Mondi <[email protected]> Reviewed-by: Laurent Pinchart <[email protected]>
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 config validation is already performed in pisp_be_validate_config() and while job.hw_enables is modified by pispbe_xlate_addrs(), the function only resets the input masks if - there is no input buffer available, but pispbe_prepare_job() fails before calling pispbe_xlate_addrs() in this case - bayer_enable is 0, but in this case rgb_enable is valid as guaranteed by pisp_be_validate_config() - only outputs are reset in rgb_enable For this reasons there is no need to repeat the check 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 <[email protected]> Reviewed-by: Laurent Pinchart <[email protected]>
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. As the per-node 'ready_queue' buffer list is only accessed in vb2 ops callbacks, protected by a mutex, it is not necessary to guard it with a dedicated spinlock so drop it. Also use the spin_lock_irq() variant in all functions not called from an IRQ context where the spin_lock_irqsave() version was used. Signed-off-by: Jacopo Mondi <[email protected]>
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 but 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 suspending the interface using pm_runtime_idle() which doesn't decrease the pm_runtime usage count and inform the PM framework that the device is active by calling pm_runtime_set_active(). Adjust the pispbe_remove() function as well to disable the pm_runtime in the correct order. Signed-off-by: Jacopo Mondi <[email protected]> Reviewed-by: Laurent Pinchart <[email protected]> --- v4->v5: - Indent with tabs :/ v3->v4: - Instead of using pm_runtime for resuming, suspend using pm_runtime_idle() to support !CONFIG_PM v2->v3: - Mark pispbe_runtime_resume() as __maybe_unused as reported by the kernel test robot <[email protected]>
jmondi
force-pushed
the
pispbe/split-jobs-handling
branch
from
1114c40
to
775912b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before submitting the same change to mainline, I would like to know what you think about this. This change will make it easier to support multi-context handling without duplicating the media graph instances.
Currently the 'pispbe_schedule()' function does two things:
to make sure all the required buffers are available
The pispbe_schedule() function is called at:
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.