Hi Laurent, Mauro,
I've cast my eyes through this, and the driver code it affects
On 27/07/18 18:19, Laurent Pinchart wrote:
> The VSP uses a lock to protect the BRU and BRS assignment when
> configuring pipelines. The lock is taken in vsp1_du_atomic_begin() and
> released in vsp1_du_atomic_flush(), as well as taken and released in
> vsp1_du_setup_lif(). This guards against multiple pipelines trying to
> assign the same BRU and BRS at the same time.
>
> The DRM framework calls the .atomic_begin() operations in a loop over
> all CRTCs included in an atomic commit. On a VSPDL (the only VSP type
> where this matters), a single VSP instance handles two CRTCs, with a
> single lock. This results in a deadlock when the .atomic_begin()
> operation is called on the second CRTC.
>
> The DRM framework serializes atomic commits that affect the same CRTCs,
> but doesn't know about two CRTCs sharing the same VSPDL. Two commits
> affecting the VSPDL LIF0 and LIF1 respectively can thus race each other,
> hence the need for a lock.
>
> This could be fixed on the DRM side by forcing serialization of commits
> affecting CRTCs backed by the same VSPDL, but that would negatively
> affect performances, as the locking is only needed when the BRU and BRS
> need to be reassigned, which is an uncommon case.
>
> The lock protects the whole .atomic_begin() to .atomic_flush() sequence.
> The only operation that can occur in-between is vsp1_du_atomic_update(),
> which doesn't touch the BRU and BRS, and thus doesn't need to be
> protected by the lock. We can thus only take the lock around the
And I almost replied to say ... but what about vsp1_du_atomic_update()
before re-reading this paragraph :)
> pipeline setup calls in vsp1_du_atomic_flush(), which fixes the
> deadlock.
>
> Fixes: f81f9adc4ee1 ("media: v4l: vsp1: Assign BRU and BRS to pipelines dynamically")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
It makes me very happy to see the lock/unlock across separate functions
removed :)
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/vsp1/vsp1_drm.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> I've successfully tested the patch with kmstest --flip running with four
> outputs on a Salvator-XS board, as well as with the DU kms-test-brxalloc.py
> test. The deadlock is gone, and no race has been observed.
>
> Mauro, this is a v4.18 regression fix. I'm sorry for sending it so late,
> I haven't noticed the issue earlier. Once Kieran reviews it (which should
> happen in the next few days), could you send it to Linus ? The breakage is
> pretty bad otherwise for people using both the VGA and LVDS outputs at the
> same time.
>
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index edb35a5c57ea..a99fc0ced7a7 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -728,9 +728,6 @@ EXPORT_SYMBOL_GPL(vsp1_du_setup_lif);
> */
> void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index)
> {
> - struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> -
> - mutex_lock(&vsp1->drm->lock);
> }
> EXPORT_SYMBOL_GPL(vsp1_du_atomic_begin);
>
> @@ -846,6 +843,7 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
>
> drm_pipe->crc = cfg->crc;
>
> + mutex_lock(&vsp1->drm->lock);
> vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> vsp1_du_pipeline_configure(pipe);
> mutex_unlock(&vsp1->drm->lock);
>
--
Regards
--
Kieran
