[PATCH 11/15] v4l: vsp1: Add per-display list completion notification support

[Kieran Bingham]

Hi Laurent,

On 04/04/18 22:43, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wednesday, 4 April 2018 19:16:46 EEST Kieran Bingham wrote:
>> On 26/02/18 21:45, Laurent Pinchart wrote:

<snip>

>>>
>>> -void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>>> +void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool notify)
>>
>> Rather than changing the vsp1_dl_list_commit() function - would it be nicer
>> to have an API to request or set the notify property? :
>>
>> @@..@@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>> ...
>> +	/* The BRx will be released, without sending an update to DRM */
>> +	if (drm_pipe->force_bru_release)
>> +		vsp1_dl_list_request_internal_notify(dl);
>>
>> 	vsp1_dl_list_commit(dl);
>> ...
> 
> That's not a bad idea, but I wonder if it's worth it as we'll have to call an 
> extra function for what is essentially an internal API. On the other hand this 
> isn't a common case, so it's not a hot code path. We could also argue equally 
> that it is the commit that is internal or that it is the display list that is 

Aha, yes - it is more so that the commit is internal ...

> for internal purpose. Do you think an extra function call is better ? If you 
> do I'll change it.

so it could also instead be just a separate commit() function:


void vsp1_dl_list_commit_internal(struct vsp1_dl_list *dl)
{
	dl->internal = true;
	vsp1_dl_list_commit(dl);
}


...


{
	/* The BRx will be released, without sending an update to DRM */
	if (drm_pipe->force_bru_release)
		vsp1_dl_list_commit_internal(dl);
	else
 		vsp1_dl_list_commit(dl);
}

I'll leave the final implementation decision with you - I just thought that
extending the commit call with a notify flag seemed odd.

Of course - that could also have been due to the naming of the 'notify' - if it
was 'internal' as discussed in the other patches, then perhaps a flag on the
function call is still a sensible way. It just affects the other commit usages,
but there's only a total of three calls - so it's really not a big deal. If
there were 12 call locations perhaps the function wrapper would have more merit
- but probably not so much at 3 :D

--
Kieran