[RFC PATCH 1/3] v4l: vsp1: Register pipe with output WPF

[Kieran Bingham]

Hi Laurent,

Thanks for the review,

On 03/03/17 01:57, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 01 Mar 2017 13:12:54 Kieran Bingham wrote:
>> The DRM object does not register the pipe with the WPF object. This is
>> used internally throughout the driver as a means of accessing the pipe.
>> As such this breaks operations which require access to the pipe from WPF
>> interrupts.
>>
>> Register the pipe inside the WPF object after it has been declared as
>> the output.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas at ideasonboard.com>
>> ---
>>  drivers/media/platform/vsp1/vsp1_drm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
>> b/drivers/media/platform/vsp1/vsp1_drm.c index cd209dccff1b..8e2aa3f8e52f
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>> @@ -596,6 +596,7 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
>>  	pipe->bru = &vsp1->bru->entity;
>>  	pipe->lif = &vsp1->lif->entity;
>>  	pipe->output = vsp1->wpf[0];
>> +	pipe->output->pipe = pipe;
> 
> The vsp1_irq_handler() function calls vsp1_pipeline_frame_end() with wpf-
>> pipe, which is currently NULL. With this patch the function will get a non-
> NULL pipeline and will thus proceed to calling vsp1_dlm_irq_frame_end():
> 
> void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
> {
> 	if (pipe == NULL)
> 		return;
> 
> 	vsp1_dlm_irq_frame_end(pipe->output->dlm);
> 
> 	if (pipe->frame_end)
> 		pipe->frame_end(pipe);
> 
> 	pipe->sequence++;
> }
> 
> pipe->frame_end is NULL, pipe->sequence doesn't matter, but we now end up 
> calling vsp1_dlm_irq_frame_end(). This is a major change regarding display 
> list processing, yet it seems to have no effect at all.

I was a bit surprised at this bit. I had expected vsp1_dlm_irq_frame_end() to be
more critical, but ultimately - it's only job is to handle a race condition on
DL commits which we (presumably) don't hit. At least we don't hit in our testing
more than $(DL_QTY) times, or I believe the display would have hung.

In fact we would have panic'd on a NULL dereference with pipe->dl:
	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);

where I don't see any checks on pipe->dl != NULL in the current code paths.

> The following commit is to blame for skipping the call to 
> vsp1_dlm_irq_frame_end().
> 
> commit ff7e97c94d9f7f370fe3ce2a72e85361ca22a605
> Author: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>
> Date:   Tue Jan 19 19:16:36 2016 -0200
> 
>     [media] v4l: vsp1: Store pipeline pointer in rwpf
> 
> I've added a few debug print statements to vsp1_dlm_irq_frame_end(), and it 
> looks like we only hit the if (dlm->queued) test or none of them at all. It 
> looks like we've been lucky that nothing broke.
> 
> Restoring the previous behaviour should be safe, but it would be worth it 
> inspecting the code very carefully to make sure the logic is still correct. 
> I'll do it tomorrow if you don't beat me to it.

As far as I can tell it still seems correct. If we had ever hit this race, I
believe we would have leaked the DL, (which is quite a limited resource to us),
which if anything just raises the importance of this fix.

> 
> In any case, how about adding a
> 
> Fixes: ff7e97c94d9f ("[media] v4l: vsp1: Store pipeline pointer in rwpf")

Absolutely.

This seems worthy of a stable backport...? (though not critical)

The breakage was introduced in 4.6...

Should I add a CC: stable at kernel.org #4.6+

> line ?
> 
>>  	return 0;
>>  }
> 

-- 
Regards

Kieran Bingham