[PATCH] media: vsp1: Simplify DRM UIF handling

[PATCH] media: vsp1: Simplify DRM UIF handling

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

In commit 6732f3139380 ("media: v4l: vsp1: Fix uif null pointer access")
the handling of the UIF was over complicated, and the patch applied
before review.

Simplify it to keep the conditionals small.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---

Another one that I had lying around in my tree for too long ....

 drivers/media/platform/vsp1/vsp1_drm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 06f74d410973..0c2507dc03d6 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -455,6 +455,10 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
 		dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
 			__func__, BRX_NAME(pipe->brx));
 
+	/* If the DRM pipe does not have a UIF there is nothing we can update. */
+	if (!drm_pipe->uif)
+		return 0;
+
 	/*
 	 * If the UIF is not in use schedule it for removal by setting its pipe
 	 * pointer to NULL, vsp1_du_pipeline_configure() will remove it from the
@@ -462,9 +466,9 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
 	 * make sure it is present in the pipeline's list of entities if it
 	 * wasn't already.
 	 */
-	if (drm_pipe->uif && !use_uif) {
+	if (!use_uif) {
 		drm_pipe->uif->pipe = NULL;
-	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {
+	} else if (!drm_pipe->uif->pipe) {
 		drm_pipe->uif->pipe = pipe;
 		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
 	}
-- 
2.30.2

Re: [PATCH] media: vsp1: Simplify DRM UIF handling

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Jun 18, 2021 at 08:09:05PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> In commit 6732f3139380 ("media: v4l: vsp1: Fix uif null pointer access")
> the handling of the UIF was over complicated, and the patch applied
> before review.
> 
> Simplify it to keep the conditionals small.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> 
> Another one that I had lying around in my tree for too long ....

It seems to have survived bitrot quite well.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  drivers/media/platform/vsp1/vsp1_drm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 06f74d410973..0c2507dc03d6 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -455,6 +455,10 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
>  		dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
>  			__func__, BRX_NAME(pipe->brx));
>  
> +	/* If the DRM pipe does not have a UIF there is nothing we can update. */
> +	if (!drm_pipe->uif)
> +		return 0;
> +
>  	/*
>  	 * If the UIF is not in use schedule it for removal by setting its pipe
>  	 * pointer to NULL, vsp1_du_pipeline_configure() will remove it from the
> @@ -462,9 +466,9 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
>  	 * make sure it is present in the pipeline's list of entities if it
>  	 * wasn't already.
>  	 */
> -	if (drm_pipe->uif && !use_uif) {
> +	if (!use_uif) {
>  		drm_pipe->uif->pipe = NULL;
> -	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {
> +	} else if (!drm_pipe->uif->pipe) {
>  		drm_pipe->uif->pipe = pipe;
>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
>  	}

-- 
Regards,

Laurent Pinchart

RE: [PATCH] media: vsp1: Simplify DRM UIF handling

[Biju Das]

Hi Kieran,

Thanks for the patch.

> Subject: [PATCH] media: vsp1: Simplify DRM UIF handling
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> In commit 6732f3139380 ("media: v4l: vsp1: Fix uif null pointer access")
> the handling of the UIF was over complicated, and the patch applied before
> review.
> 
> Simplify it to keep the conditionals small.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

I forgot this. Thanks for taking care this.

Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju

> ---
> 
> Another one that I had lying around in my tree for too long ....
> 
>  drivers/media/platform/vsp1/vsp1_drm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 06f74d410973..0c2507dc03d6 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -455,6 +455,10 @@ static int vsp1_du_pipeline_setup_inputs(struct
> vsp1_device *vsp1,
>  		dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
>  			__func__, BRX_NAME(pipe->brx));
> 
> +	/* If the DRM pipe does not have a UIF there is nothing we can
> update. */
> +	if (!drm_pipe->uif)
> +		return 0;
> +
>  	/*
>  	 * If the UIF is not in use schedule it for removal by setting its
> pipe
>  	 * pointer to NULL, vsp1_du_pipeline_configure() will remove it from
> the @@ -462,9 +466,9 @@ static int vsp1_du_pipeline_setup_inputs(struct
> vsp1_device *vsp1,
>  	 * make sure it is present in the pipeline's list of entities if it
>  	 * wasn't already.
>  	 */
> -	if (drm_pipe->uif && !use_uif) {
> +	if (!use_uif) {
>  		drm_pipe->uif->pipe = NULL;
> -	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {
> +	} else if (!drm_pipe->uif->pipe) {
>  		drm_pipe->uif->pipe = pipe;
>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
>  	}
> --
> 2.30.2

Re: [PATCH] media: vsp1: Simplify DRM UIF handling

[Kieran Bingham]

On 18/06/2021 20:33, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Jun 18, 2021 at 08:09:05PM +0100, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> In commit 6732f3139380 ("media: v4l: vsp1: Fix uif null pointer access")
>> the handling of the UIF was over complicated, and the patch applied
>> before review.
>>
>> Simplify it to keep the conditionals small.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>
>> Another one that I had lying around in my tree for too long ....
> 
> It seems to have survived bitrot quite well.

This still applies with git am to my tree here ...

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, do I need to do anything else to progress this ?

--
Kieran


> 
>>  drivers/media/platform/vsp1/vsp1_drm.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
>> index 06f74d410973..0c2507dc03d6 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>> @@ -455,6 +455,10 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
>>  		dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
>>  			__func__, BRX_NAME(pipe->brx));
>>  
>> +	/* If the DRM pipe does not have a UIF there is nothing we can update. */
>> +	if (!drm_pipe->uif)
>> +		return 0;
>> +
>>  	/*
>>  	 * If the UIF is not in use schedule it for removal by setting its pipe
>>  	 * pointer to NULL, vsp1_du_pipeline_configure() will remove it from the
>> @@ -462,9 +466,9 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
>>  	 * make sure it is present in the pipeline's list of entities if it
>>  	 * wasn't already.
>>  	 */
>> -	if (drm_pipe->uif && !use_uif) {
>> +	if (!use_uif) {
>>  		drm_pipe->uif->pipe = NULL;
>> -	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {
>> +	} else if (!drm_pipe->uif->pipe) {
>>  		drm_pipe->uif->pipe = pipe;
>>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
>>  	}
> 

Re: [PATCH] media: vsp1: Simplify DRM UIF handling

[Laurent Pinchart]

Hi Kieran,

On Wed, Sep 01, 2021 at 11:07:40PM +0100, Kieran Bingham wrote:
> On 18/06/2021 20:33, Laurent Pinchart wrote:
> > On Fri, Jun 18, 2021 at 08:09:05PM +0100, Kieran Bingham wrote:
> >> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>
> >> In commit 6732f3139380 ("media: v4l: vsp1: Fix uif null pointer access")
> >> the handling of the UIF was over complicated, and the patch applied
> >> before review.
> >>
> >> Simplify it to keep the conditionals small.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >>
> >> Another one that I had lying around in my tree for too long ....
> > 
> > It seems to have survived bitrot quite well.
> 
> This still applies with git am to my tree here ...
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks, do I need to do anything else to progress this ?

You can ping me to remind me to send a pull request. Which you just did
:-)

> >>  drivers/media/platform/vsp1/vsp1_drm.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> >> index 06f74d410973..0c2507dc03d6 100644
> >> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> >> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> >> @@ -455,6 +455,10 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
> >>  		dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
> >>  			__func__, BRX_NAME(pipe->brx));
> >>  
> >> +	/* If the DRM pipe does not have a UIF there is nothing we can update. */
> >> +	if (!drm_pipe->uif)
> >> +		return 0;
> >> +
> >>  	/*
> >>  	 * If the UIF is not in use schedule it for removal by setting its pipe
> >>  	 * pointer to NULL, vsp1_du_pipeline_configure() will remove it from the
> >> @@ -462,9 +466,9 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
> >>  	 * make sure it is present in the pipeline's list of entities if it
> >>  	 * wasn't already.
> >>  	 */
> >> -	if (drm_pipe->uif && !use_uif) {
> >> +	if (!use_uif) {
> >>  		drm_pipe->uif->pipe = NULL;
> >> -	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {
> >> +	} else if (!drm_pipe->uif->pipe) {
> >>  		drm_pipe->uif->pipe = pipe;
> >>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
> >>  	}

-- 
Regards,

Laurent Pinchart