Re: [PATCH 3/6] media: vsp1: dl: Use singleshot DL for VSPX

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Thu, Jan 23, 2025 at 06:04:04PM +0100, Jacopo Mondi wrote:
> The vsp1_dl library allows to program a display list and feed it
> continuously to the VSP2. As an alternative operation mode, the library
> allows to program the VSP2 in 'single shot' mode, where a display list
> is submitted to the VSP on request only.
> 
> Currently the 'single shot' mode is only available when the VSP2 is
> controlled by userspace, while it works in continuous mode when driven
> by DRM, as frames have to be submitted to the display continuously.
> 
> For the VSPX use case, where there is no uapi support, we should however
> work in single-shot mode as the ISP driver programs the VSPX on
> request.
> 
> Initialize the display lists in single shot mode in case the VSP1 is
> controlled by userspace or in case the pipeline features an IIF entity,
> as in the VSPX case.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/vsp1/vsp1_dl.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_dl.c b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> index ad3fa1c9cc737c92870c087dd7cb8cf584fce41b..b8f0398522257f2fb771b419f34b56e59707476b 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_dl.c
> @@ -1099,7 +1099,12 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
>  		return NULL;
>  
>  	dlm->index = index;
> -	dlm->singleshot = vsp1->info->uapi;
> +	/*
> +	 * uapi = single shot mode;
> +	 * DRM = continuous mode;
> +	 * VSPX = single shot mode;
> +	 */
> +	dlm->singleshot = (vsp1->info->uapi || vsp1->iif);

I'm wondering if we should make this a bit more generic, and allow the
caller to select the mode of operation. It could be passed as a flag to
vsp1_dl_list_commit() and stored in the vsp1_dl_list.

There is however no use case at the moment to switch between singleshot
and continuous modes on a per display list basis. Implementing support
for that may be overkill, but on the other hand, the implementation
seems very simple, so it's not a big effort. The main and possibly only
reason why we may not want to do that now is because the display list
helpers haven't been tested to successfully switch between the modes, so
we may falsely advertise something as supported. Despite that, as no
caller would attempt switching, it wouldn't cause an issue.

What do you think ? What do you feel would be cleaner ?

>  	dlm->vsp1 = vsp1;
>  
>  	spin_lock_init(&dlm->lock);

-- 
Regards,

Laurent Pinchart