[PATCH 0/2] VSP trivial enhancements

[PATCH 0/2] VSP trivial enhancements

[Biju Das]

This patch series fixes 2 null pointer issues on the below cases

1) Use BRS without BRU module

2) UIF module is not used.

This issues were found during display bringup on RZ/G2L SoC.
RZ/G2L has only BRS module and no UIF module.

Biju Das (2):
  media: v4l: vsp1: Fix bru null pointer access
  media: v4l: vsp1: Fix uif null pointer access

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

-- 
2.17.1

[PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access

[Biju Das]

RZ/G2L SoC has only BRS. This patch fixes null pointer access,when only
BRS is enabled.

Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 86d5e3f4b1ff..f6d2f47a4058 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
 		brx = &vsp1->bru->entity;
 	else if (pipe->brx && !drm_pipe->force_brx_release)
 		brx = pipe->brx;
-	else if (!vsp1->bru->entity.pipe)
+	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) && !vsp1->bru->entity.pipe)
 		brx = &vsp1->bru->entity;
 	else
 		brx = &vsp1->brs->entity;
-- 
2.17.1

[PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Biju Das]

RZ/G2L SoC has no UIF. This patch fixes null pointer access, when UIF
module is not used.

Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display pipeline")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index f6d2f47a4058..06f74d410973 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -462,9 +462,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 (!use_uif) {
+	if (drm_pipe->uif && !use_uif) {
 		drm_pipe->uif->pipe = NULL;
-	} else if (!drm_pipe->uif->pipe) {
+	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {
 		drm_pipe->uif->pipe = pipe;
 		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
 	}
-- 
2.17.1

Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Kieran Bingham]

Hi Biju,

On 01/03/2021 12:08, Biju Das wrote:
> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when UIF
> module is not used.
> 
> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display pipeline")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index f6d2f47a4058..06f74d410973 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,


This looks like it complicates these conditionals more than we perhaps
need to.

What do you think about adding something above the block comment here?:

	if (!drm_pipe->uif)
		return 0;


>  	 * make sure it is present in the pipeline's list of entities if it
>  	 * wasn't already.
>  	 */
> -	if (!use_uif) {
> +	if (drm_pipe->uif && !use_uif) {
>  		drm_pipe->uif->pipe = NULL;
> -	} else if (!drm_pipe->uif->pipe) {
> +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {>  		drm_pipe->uif->pipe = pipe;
>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
>  	}
> 

Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access

[Kieran Bingham]

Hi Biju,

On 01/03/2021 12:08, Biju Das wrote:
> RZ/G2L SoC has only BRS. This patch fixes null pointer access,when only
> BRS is enabled.
> 
> Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 86d5e3f4b1ff..f6d2f47a4058 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
>  		brx = &vsp1->bru->entity;
>  	else if (pipe->brx && !drm_pipe->force_brx_release)
>  		brx = pipe->brx;
> -	else if (!vsp1->bru->entity.pipe)
> +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) && !vsp1->bru->entity.pipe)
>  		brx = &vsp1->bru->entity;
>  	else
>  		brx = &vsp1->brs->entity;


The comments here describe that the choice to start at the BRU is
arbitrary, so if we could confirm that there will always be a BRS
otherwise, we could swap those to save an extra feature check.

But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.

I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
brs->entity line to keep the symmetry ... but it wouldn't be needed, as
it should fall through. If there isn't a BRS there must be a BRU or we
wouldn't be setting up a brx in the first place ;-)

So I think what you have is good.

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

RE: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Biju Das]

Hi Kieran,

Thanks for the feedback.


> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access
> 
> Hi Biju,
> 
> On 01/03/2021 12:08, Biju Das wrote:
> > RZ/G2L SoC has no UIF. This patch fixes null pointer access, when UIF
> > module is not used.
> >
> > Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display
> > pipeline")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c
> > index f6d2f47a4058..06f74d410973 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct
> > vsp1_device *vsp1,
> 
> 
> This looks like it complicates these conditionals more than we perhaps
> need to.
> 
> What do you think about adding something above the block comment here?:

It is much better. 

This patch is accepted in media tree[1]. So not sure,
should I send a follow up patch as optimization or drop this patch and send new one.

Please suggest.

[1] https://git.linuxtv.org/media_tree.git/commit/?h=fixes&id=c4f27003ec3d84ef0c333c74ae2aff326537e583

Cheers,
Biju

> 	if (!drm_pipe->uif)
> 		return 0;
> 
> 
> >  	 * make sure it is present in the pipeline's list of entities if it
> >  	 * wasn't already.
> >  	 */
> > -	if (!use_uif) {
> > +	if (drm_pipe->uif && !use_uif) {
> >  		drm_pipe->uif->pipe = NULL;
> > -	} else if (!drm_pipe->uif->pipe) {
> > +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {>
> 	drm_pipe->uif->pipe = pipe;
> >  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
> >  	}
> >

Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Kieran Bingham]

Hi Biju,

On 10/03/2021 13:56, Biju Das wrote:
> Hi Kieran,
> 
> Thanks for the feedback.
>> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access
>>
>> Hi Biju,
>>
>> On 01/03/2021 12:08, Biju Das wrote:
>>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when UIF
>>> module is not used.
>>>
>>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display
>>> pipeline")
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> ---
>>>  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
>>> b/drivers/media/platform/vsp1/vsp1_drm.c
>>> index f6d2f47a4058..06f74d410973 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct
>>> vsp1_device *vsp1,
>>
>>
>> This looks like it complicates these conditionals more than we perhaps
>> need to.
>>
>> What do you think about adding something above the block comment here?:
> 
> It is much better. 
> 
> This patch is accepted in media tree[1]. So not sure,
> should I send a follow up patch as optimization or drop this patch and send new one.

Oh, I didn't realise these were in already. Sorry, I didn't see any
review on the list, and it was the earliest I had got to them.

> Please suggest.

Up to you, I don't think this would get dropped now it's integrated.
It's in, so if you want to update on top I believe that's fine.

--
Kieran


> [1] https://git.linuxtv.org/media_tree.git/commit/?h=fixes&id=c4f27003ec3d84ef0c333c74ae2aff326537e583
> 
> Cheers,
> Biju
> 
>> 	if (!drm_pipe->uif)
>> 		return 0;
>>
>>
>>>  	 * make sure it is present in the pipeline's list of entities if it
>>>  	 * wasn't already.
>>>  	 */
>>> -	if (!use_uif) {
>>> +	if (drm_pipe->uif && !use_uif) {
>>>  		drm_pipe->uif->pipe = NULL;
>>> -	} else if (!drm_pipe->uif->pipe) {
>>> +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {>
>> 	drm_pipe->uif->pipe = pipe;
>>>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
>>>  	}
>>>
> 

RE: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Biju Das]

Hi Kieran,

> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access
> 
> Hi Biju,
> 
> On 10/03/2021 13:56, Biju Das wrote:
> > Hi Kieran,
> >
> > Thanks for the feedback.
> >> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer
> >> access
> >>
> >> Hi Biju,
> >>
> >> On 01/03/2021 12:08, Biju Das wrote:
> >>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when
> >>> UIF module is not used.
> >>>
> >>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display
> >>> pipeline")
> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> ---
> >>>  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> >>> b/drivers/media/platform/vsp1/vsp1_drm.c
> >>> index f6d2f47a4058..06f74d410973 100644
> >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> >>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct
> >>> vsp1_device *vsp1,
> >>
> >>
> >> This looks like it complicates these conditionals more than we
> >> perhaps need to.
> >>
> >> What do you think about adding something above the block comment here?:
> >
> > It is much better.
> >
> > This patch is accepted in media tree[1]. So not sure, should I send a
> > follow up patch as optimization or drop this patch and send new one.
> 
> Oh, I didn't realise these were in already. Sorry, I didn't see any review
> on the list, and it was the earliest I had got to them.
> 
> > Please suggest.
> 
> Up to you, I don't think this would get dropped now it's integrated.
> It's in, so if you want to update on top I believe that's fine.

OK, Will send follow up patch as optimization.

Cheers,
Biju

> --
> Kieran
> 
> 
> > [1]
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> > linuxtv.org%2Fmedia_tree.git%2Fcommit%2F%3Fh%3Dfixes%26id%3Dc4f27003ec
> > 3d84ef0c333c74ae2aff326537e583&amp;data=04%7C01%7Cbiju.das.jz%40bp.ren
> > esas.com%7Cad7bafadc61345db66e908d8e3d24f3c%7C53d82571da1947e49cb4625a
> > 166a4a2a%7C0%7C0%7C637509839726919053%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;
> > sdata=jb%2BzwAJD6%2FxBKsQEryhDeKnJbgI%2F3RNZCIXfYU4EBCw%3D&amp;reserve
> > d=0
> >
> > Cheers,
> > Biju
> >
> >> 	if (!drm_pipe->uif)
> >> 		return 0;
> >>
> >>
> >>>  	 * make sure it is present in the pipeline's list of entities if it
> >>>  	 * wasn't already.
> >>>  	 */
> >>> -	if (!use_uif) {
> >>> +	if (drm_pipe->uif && !use_uif) {
> >>>  		drm_pipe->uif->pipe = NULL;
> >>> -	} else if (!drm_pipe->uif->pipe) {
> >>> +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {>
> >> 	drm_pipe->uif->pipe = pipe;
> >>>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
> >>>  	}
> >>>
> >

RE: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access

[Biju Das]

Hi Kieran,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access
> 
> Hi Biju,
> 
> On 01/03/2021 12:08, Biju Das wrote:
> > RZ/G2L SoC has only BRS. This patch fixes null pointer access,when
> > only BRS is enabled.
> >
> > Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 86d5e3f4b1ff..f6d2f47a4058 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> vsp1_device *vsp1,
> >  		brx = &vsp1->bru->entity;
> >  	else if (pipe->brx && !drm_pipe->force_brx_release)
> >  		brx = pipe->brx;
> > -	else if (!vsp1->bru->entity.pipe)
> > +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) &&
> > +!vsp1->bru->entity.pipe)
> >  		brx = &vsp1->bru->entity;
> >  	else
> >  		brx = &vsp1->brs->entity;
> 
> 
> The comments here describe that the choice to start at the BRU is
> arbitrary, so if we could confirm that there will always be a BRS
> otherwise, we could swap those to save an extra feature check.

As long as we are supporting composition(Multiple inputs with Blend and Raster operations)
There will be either BRU or BRS or both in R-Car Gen3|RZ/G2 SoC's. Currently this is
the case with all SoC variant of this families.

Cheers,
Biju

> 
> But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
> vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.
> 
> I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
> brs->entity line to keep the symmetry ... but it wouldn't be needed, as
> it should fall through. If there isn't a BRS there must be a BRU or we
> wouldn't be setting up a brx in the first place ;-)
> 
> So I think what you have is good.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access

[Laurent Pinchart]

Hi Biju,

On Thu, Mar 11, 2021 at 07:15:01AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access
> > 
> > Hi Biju,
> > 
> > On 01/03/2021 12:08, Biju Das wrote:
> > > RZ/G2L SoC has only BRS. This patch fixes null pointer access,when
> > > only BRS is enabled.
> > >
> > > Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")

Given that RZ/G2L isn't supported in mainline, this is hardly a fix, is
it ?

> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > > b/drivers/media/platform/vsp1/vsp1_drm.c
> > > index 86d5e3f4b1ff..f6d2f47a4058 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > > @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> > vsp1_device *vsp1,
> > >  		brx = &vsp1->bru->entity;
> > >  	else if (pipe->brx && !drm_pipe->force_brx_release)
> > >  		brx = pipe->brx;
> > > -	else if (!vsp1->bru->entity.pipe)
> > > +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) &&
> > > +!vsp1->bru->entity.pipe)
> > >  		brx = &vsp1->bru->entity;
> > >  	else
> > >  		brx = &vsp1->brs->entity;
> > 
> > 
> > The comments here describe that the choice to start at the BRU is
> > arbitrary, so if we could confirm that there will always be a BRS
> > otherwise, we could swap those to save an extra feature check.
> 
> As long as we are supporting composition(Multiple inputs with Blend and Raster operations)
> There will be either BRU or BRS or both in R-Car Gen3|RZ/G2 SoC's. Currently this is
> the case with all SoC variant of this families.

Given that the function is called vsp1_du_pipeline_setup_brx(), I think
we can assume there will be either a BRU or a BRS :-)

How many RPF instances does the RG/G2L VSPD have ?

> > But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
> > vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.
> > 
> > I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
> > brs->entity line to keep the symmetry ... but it wouldn't be needed, as
> > it should fall through. If there isn't a BRS there must be a BRU or we
> > wouldn't be setting up a brx in the first place ;-)
> > 
> > So I think what you have is good.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Laurent Pinchart]

Hi Biju,

On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote:
> > On 10/03/2021 13:56, Biju Das wrote:
> > > Thanks for the feedback.
> > >> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer
> > >> access
> > >>
> > >> Hi Biju,
> > >>
> > >> On 01/03/2021 12:08, Biju Das wrote:
> > >>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when
> > >>> UIF module is not used.
> > >>>
> > >>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display
> > >>> pipeline")
> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >>> ---
> > >>>  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> > >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > >>> b/drivers/media/platform/vsp1/vsp1_drm.c
> > >>> index f6d2f47a4058..06f74d410973 100644
> > >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > >>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct
> > >>> vsp1_device *vsp1,
> > >>
> > >>
> > >> This looks like it complicates these conditionals more than we
> > >> perhaps need to.
> > >>
> > >> What do you think about adding something above the block comment here?:
> > >
> > > It is much better.
> > >
> > > This patch is accepted in media tree[1]. So not sure, should I send a
> > > follow up patch as optimization or drop this patch and send new one.
> > 
> > Oh, I didn't realise these were in already. Sorry, I didn't see any review
> > on the list, and it was the earliest I had got to them.
> > 
> > > Please suggest.
> > 
> > Up to you, I don't think this would get dropped now it's integrated.
> > It's in, so if you want to update on top I believe that's fine.
> 
> OK, Will send follow up patch as optimization.

That would be nice.

I don't think this patch should have been fast-tracked as a fix, as
RZ/G2L isn't supported in mainline yet as far as I can tell.

Hans, next time, could we get a notification instead of a silent merge ?

> > >> 	if (!drm_pipe->uif)
> > >> 		return 0;
> > >>
> > >>
> > >>>  	 * make sure it is present in the pipeline's list of entities if it
> > >>>  	 * wasn't already.
> > >>>  	 */
> > >>> -	if (!use_uif) {
> > >>> +	if (drm_pipe->uif && !use_uif) {
> > >>>  		drm_pipe->uif->pipe = NULL;
> > >>> -	} else if (!drm_pipe->uif->pipe) {
> > >>> +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {>
> > >> 	drm_pipe->uif->pipe = pipe;
> > >>>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
> > >>>  	}
> > >>>

-- 
Regards,

Laurent Pinchart

RE: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 15 March 2021 03:43
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: kieran.bingham+renesas@ideasonboard.com; Mauro Carvalho Chehab
> <mchehab@kernel.org>; linux-media@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>; Chris
> Paterson <Chris.Paterson2@renesas.com>; Biju Das
> <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access
> 
> Hi Biju,
> 
> On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote:
> > > On 10/03/2021 13:56, Biju Das wrote:
> > > > Thanks for the feedback.
> > > >> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer
> > > >> access
> > > >>
> > > >> Hi Biju,
> > > >>
> > > >> On 01/03/2021 12:08, Biju Das wrote:
> > > >>> RZ/G2L SoC has no UIF. This patch fixes null pointer access,
> > > >>> when UIF module is not used.
> > > >>>
> > > >>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in
> > > >>> display
> > > >>> pipeline")
> > > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > >>> ---
> > > >>>  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> > > >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > > >>> b/drivers/media/platform/vsp1/vsp1_drm.c
> > > >>> index f6d2f47a4058..06f74d410973 100644
> > > >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > > >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > > >>> @@ -462,9 +462,9 @@ static int
> > > >>> vsp1_du_pipeline_setup_inputs(struct
> > > >>> vsp1_device *vsp1,
> > > >>
> > > >>
> > > >> This looks like it complicates these conditionals more than we
> > > >> perhaps need to.
> > > >>
> > > >> What do you think about adding something above the block comment
> here?:
> > > >
> > > > It is much better.
> > > >
> > > > This patch is accepted in media tree[1]. So not sure, should I
> > > > send a follow up patch as optimization or drop this patch and send
> new one.
> > >
> > > Oh, I didn't realise these were in already. Sorry, I didn't see any
> > > review on the list, and it was the earliest I had got to them.
> > >
> > > > Please suggest.
> > >
> > > Up to you, I don't think this would get dropped now it's integrated.
> > > It's in, so if you want to update on top I believe that's fine.
> >
> > OK, Will send follow up patch as optimization.
> 
> That would be nice.
> 
> I don't think this patch should have been fast-tracked as a fix, as RZ/G2L
> isn't supported in mainline yet as far as I can tell.


Please correct me, if I am wrong. We have pluggable modules in VSP and with routing
Register we can achieve any combination with the VSP driver we have. 

If it is the case, it is a bug which is fast-tracked as a fix. Otherwise(ie, driver doesn't have flexibility to support pluggable feature) I am agreeing with your statement.

Cheers,
Biju

> Hans, next time, could we get a notification instead of a silent merge ?
> 
> > > >> 	if (!drm_pipe->uif)
> > > >> 		return 0;
> > > >>
> > > >>
> > > >>>  	 * make sure it is present in the pipeline's list of entities
> if it
> > > >>>  	 * wasn't already.
> > > >>>  	 */
> > > >>> -	if (!use_uif) {
> > > >>> +	if (drm_pipe->uif && !use_uif) {
> > > >>>  		drm_pipe->uif->pipe = NULL;
> > > >>> -	} else if (!drm_pipe->uif->pipe) {
> > > >>> +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {>
> > > >> 	drm_pipe->uif->pipe = pipe;
> > > >>>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe-
> >entities);
> > > >>>  	}
> > > >>>
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access
> 
> Hi Biju,
> 
> On Thu, Mar 11, 2021 at 07:15:01AM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer
> > > access
> > >
> > > Hi Biju,
> > >
> > > On 01/03/2021 12:08, Biju Das wrote:
> > > > RZ/G2L SoC has only BRS. This patch fixes null pointer access,when
> > > > only BRS is enabled.
> > > >
> > > > Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
> 
> Given that RZ/G2L isn't supported in mainline, this is hardly a fix, is it
> ?

I agree RZ/G2L is a new SoC.

Please see my comments for other patch. I have added fixes tag based on pluggable feature on VSP driver.

If we have BRS and BRU, we can select either BRS or BRU. For eg:- VSPDL in R-Car H3/H3-N/M3-N and also R-Car V3M/V3H we have both BRS and BRU. Since both are present on this SoC's, it won't trigger the null pointer issue. 

But as new SoC's like RZ/G2L have only BRS.

> 
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > > > b/drivers/media/platform/vsp1/vsp1_drm.c
> > > > index 86d5e3f4b1ff..f6d2f47a4058 100644
> > > > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > > > @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> > > vsp1_device *vsp1,
> > > >  		brx = &vsp1->bru->entity;
> > > >  	else if (pipe->brx && !drm_pipe->force_brx_release)
> > > >  		brx = pipe->brx;
> > > > -	else if (!vsp1->bru->entity.pipe)
> > > > +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) &&
> > > > +!vsp1->bru->entity.pipe)
> > > >  		brx = &vsp1->bru->entity;
> > > >  	else
> > > >  		brx = &vsp1->brs->entity;
> > >
> > >
> > > The comments here describe that the choice to start at the BRU is
> > > arbitrary, so if we could confirm that there will always be a BRS
> > > otherwise, we could swap those to save an extra feature check.
> >
> > As long as we are supporting composition(Multiple inputs with Blend
> > and Raster operations) There will be either BRU or BRS or both in
> > R-Car Gen3|RZ/G2 SoC's. Currently this is the case with all SoC variant
> of this families.
> 
> Given that the function is called vsp1_du_pipeline_setup_brx(), I think we
> can assume there will be either a BRU or a BRS :-)
> 
> How many RPF instances does the RG/G2L VSPD have ?

2 RPF, 1WPF , 1 BRS, 1 LUT and 1 LIF

Cheers,
Biju

> 
> > > But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
> > > vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.
> > >
> > > I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
> > > brs->entity line to keep the symmetry ... but it wouldn't be needed,
> > > brs->as
> > > it should fall through. If there isn't a BRS there must be a BRU or
> > > we wouldn't be setting up a brx in the first place ;-)
> > >
> > > So I think what you have is good.
> > >
> > > Reviewed-by: Kieran Bingham
> > > <kieran.bingham+renesas@ideasonboard.com>
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access

[Laurent Pinchart]

Hi Biju,

On Mon, Mar 15, 2021 at 09:15:00AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access
> > On Thu, Mar 11, 2021 at 07:15:01AM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer
> > > > access
> > > >
> > > > Hi Biju,
> > > >
> > > > On 01/03/2021 12:08, Biju Das wrote:
> > > > > RZ/G2L SoC has only BRS. This patch fixes null pointer access,when
> > > > > only BRS is enabled.
> > > > >
> > > > > Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
> > 
> > Given that RZ/G2L isn't supported in mainline, this is hardly a fix, is it
> > ?
> 
> I agree RZ/G2L is a new SoC.
> 
> Please see my comments for other patch. I have added fixes tag based
> on pluggable feature on VSP driver.
> 
> If we have BRS and BRU, we can select either BRS or BRU. For eg:-
> VSPDL in R-Car H3/H3-N/M3-N and also R-Car V3M/V3H we have both BRS
> and BRU. Since both are present on this SoC's, it won't trigger the
> null pointer issue. 
> 
> But as new SoC's like RZ/G2L have only BRS.

I agree that this issue needs to be addressed, my point was that it
shouldn't have been merged in the -fixes branch (which will likely
result in the patch being backported to stable branches) but in the
-next branch, given that the problem it solves can't be triggered yet by
any platform supported in mainline.

> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > > > > b/drivers/media/platform/vsp1/vsp1_drm.c
> > > > > index 86d5e3f4b1ff..f6d2f47a4058 100644
> > > > > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > > > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > > > > @@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct
> > > > vsp1_device *vsp1,
> > > > >  		brx = &vsp1->bru->entity;
> > > > >  	else if (pipe->brx && !drm_pipe->force_brx_release)
> > > > >  		brx = pipe->brx;
> > > > > -	else if (!vsp1->bru->entity.pipe)
> > > > > +	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) &&
> > > > > +!vsp1->bru->entity.pipe)
> > > > >  		brx = &vsp1->bru->entity;
> > > > >  	else
> > > > >  		brx = &vsp1->brs->entity;
> > > >
> > > >
> > > > The comments here describe that the choice to start at the BRU is
> > > > arbitrary, so if we could confirm that there will always be a BRS
> > > > otherwise, we could swap those to save an extra feature check.
> > >
> > > As long as we are supporting composition(Multiple inputs with Blend
> > > and Raster operations) There will be either BRU or BRS or both in
> > > R-Car Gen3|RZ/G2 SoC's. Currently this is the case with all SoC variant
> > > of this families.
> > 
> > Given that the function is called vsp1_du_pipeline_setup_brx(), I think we
> > can assume there will be either a BRU or a BRS :-)
> > 
> > How many RPF instances does the RG/G2L VSPD have ?
> 
> 2 RPF, 1WPF , 1 BRS, 1 LUT and 1 LIF

Thank you.

> > > > But as we have both vsp1_feature(vsp1, VSP1_HAS_BRU) and
> > > > vsp1_feature(vsp1, VSP1_HAS_BRS), I don't think that's the case.
> > > >
> > > > I'd almost want to check for vsp1_feature(vsp1, VSP1_HAS_BRS) on the
> > > > brs->entity line to keep the symmetry ... but it wouldn't be needed,
> > > > brs->as
> > > > it should fall through. If there isn't a BRS there must be a BRU or
> > > > we wouldn't be setting up a brx in the first place ;-)
> > > >
> > > > So I think what you have is good.
> > > >
> > > > Reviewed-by: Kieran Bingham
> > > > <kieran.bingham+renesas@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Laurent Pinchart]

Hi Biju,

On Mon, Mar 15, 2021 at 08:21:38AM +0000, Biju Das wrote:
> On 15 March 2021 03:43, Laurent Pinchart wrote:
> > On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote:
> >>> On 10/03/2021 13:56, Biju Das wrote:
> >>>> Thanks for the feedback.
> >>>>> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer
> >>>>> access
> >>>>>
> >>>>> Hi Biju,
> >>>>>
> >>>>> On 01/03/2021 12:08, Biju Das wrote:
> >>>>>> RZ/G2L SoC has no UIF. This patch fixes null pointer access,
> >>>>>> when UIF module is not used.
> >>>>>>
> >>>>>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in
> >>>>>> display pipeline")
> >>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>>>>> ---
> >>>>>>  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> >>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> >>>>>> b/drivers/media/platform/vsp1/vsp1_drm.c
> >>>>>> index f6d2f47a4058..06f74d410973 100644
> >>>>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> >>>>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> >>>>>> @@ -462,9 +462,9 @@ static int
> >>>>>> vsp1_du_pipeline_setup_inputs(struct
> >>>>>> vsp1_device *vsp1,
> >>>>>
> >>>>>
> >>>>> This looks like it complicates these conditionals more than we
> >>>>> perhaps need to.
> >>>>>
> >>>>> What do you think about adding something above the block comment here?:
> >>>>
> >>>> It is much better.
> >>>>
> >>>> This patch is accepted in media tree[1]. So not sure, should I
> >>>> send a follow up patch as optimization or drop this patch and send new one.
> >>>
> >>> Oh, I didn't realise these were in already. Sorry, I didn't see any
> >>> review on the list, and it was the earliest I had got to them.
> >>>
> >>>> Please suggest.
> >>>
> >>> Up to you, I don't think this would get dropped now it's integrated.
> >>> It's in, so if you want to update on top I believe that's fine.
> >>
> >> OK, Will send follow up patch as optimization.
> > 
> > That would be nice.
> > 
> > I don't think this patch should have been fast-tracked as a fix, as RZ/G2L
> > isn't supported in mainline yet as far as I can tell.
> 
> Please correct me, if I am wrong. We have pluggable modules in VSP and with routing
> Register we can achieve any combination with the VSP driver we have. 
> 
> If it is the case, it is a bug which is fast-tracked as a fix.
> Otherwise(ie, driver doesn't have flexibility to support pluggable
> feature) I am agreeing with your statement.

My point was that this code is currently used only on platforms that
have a UIF, so there's should be no risk of this problem being
triggered. It should certainly be fixed to support RZ/G2L, but that's
not upstream yet.

> > Hans, next time, could we get a notification instead of a silent
> > merge ?
> > 
> >>>>> 	if (!drm_pipe->uif)
> >>>>> 		return 0;
> >>>>>
> >>>>>>  	 * make sure it is present in the pipeline's list of entities if it
> >>>>>>  	 * wasn't already.
> >>>>>>  	 */
> >>>>>> -	if (!use_uif) {
> >>>>>> +	if (drm_pipe->uif && !use_uif) {
> >>>>>>  		drm_pipe->uif->pipe = NULL;
> >>>>>> -	} else if (!drm_pipe->uif->pipe) {
> >>>>>> +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {
> >>>>>> 		drm_pipe->uif->pipe = pipe;
> >>>>>>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
> >>>>>>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Hans Verkuil]

On 15/03/2021 04:42, Laurent Pinchart wrote:
> Hi Biju,
> 
> On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote:
>>> On 10/03/2021 13:56, Biju Das wrote:
>>>> Thanks for the feedback.
>>>>> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer
>>>>> access
>>>>>
>>>>> Hi Biju,
>>>>>
>>>>> On 01/03/2021 12:08, Biju Das wrote:
>>>>>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when
>>>>>> UIF module is not used.
>>>>>>
>>>>>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display
>>>>>> pipeline")
>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>>> ---
>>>>>>  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
>>>>>> b/drivers/media/platform/vsp1/vsp1_drm.c
>>>>>> index f6d2f47a4058..06f74d410973 100644
>>>>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>>>>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>>>>>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct
>>>>>> vsp1_device *vsp1,
>>>>>
>>>>>
>>>>> This looks like it complicates these conditionals more than we
>>>>> perhaps need to.
>>>>>
>>>>> What do you think about adding something above the block comment here?:
>>>>
>>>> It is much better.
>>>>
>>>> This patch is accepted in media tree[1]. So not sure, should I send a
>>>> follow up patch as optimization or drop this patch and send new one.
>>>
>>> Oh, I didn't realise these were in already. Sorry, I didn't see any review
>>> on the list, and it was the earliest I had got to them.
>>>
>>>> Please suggest.
>>>
>>> Up to you, I don't think this would get dropped now it's integrated.
>>> It's in, so if you want to update on top I believe that's fine.
>>
>> OK, Will send follow up patch as optimization.
> 
> That would be nice.
> 
> I don't think this patch should have been fast-tracked as a fix, as
> RZ/G2L isn't supported in mainline yet as far as I can tell.
> 
> Hans, next time, could we get a notification instead of a silent merge ?

My apologies, it seemed a trivial fix, but I should have checked with you.

I jumped the gun here :-(

Regards,

	Hans

> 
>>>>> 	if (!drm_pipe->uif)
>>>>> 		return 0;
>>>>>
>>>>>
>>>>>>  	 * make sure it is present in the pipeline's list of entities if it
>>>>>>  	 * wasn't already.
>>>>>>  	 */
>>>>>> -	if (!use_uif) {
>>>>>> +	if (drm_pipe->uif && !use_uif) {
>>>>>>  		drm_pipe->uif->pipe = NULL;
>>>>>> -	} else if (!drm_pipe->uif->pipe) {
>>>>>> +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {>
>>>>> 	drm_pipe->uif->pipe = pipe;
>>>>>>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
>>>>>>  	}
>>>>>>
> 

Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access

[Laurent Pinchart]

Hi Hans,

On Tue, Mar 16, 2021 at 09:21:15AM +0100, Hans Verkuil wrote:
> On 15/03/2021 04:42, Laurent Pinchart wrote:
> > On Wed, Mar 10, 2021 at 02:50:23PM +0000, Biju Das wrote:
> >>> On 10/03/2021 13:56, Biju Das wrote:
> >>>> Thanks for the feedback.
> >>>>> Subject: Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer
> >>>>> access
> >>>>>
> >>>>> Hi Biju,
> >>>>>
> >>>>> On 01/03/2021 12:08, Biju Das wrote:
> >>>>>> RZ/G2L SoC has no UIF. This patch fixes null pointer access, when
> >>>>>> UIF module is not used.
> >>>>>>
> >>>>>> Fixes: 5e824f989e6e8("media: v4l: vsp1: Integrate DISCOM in display
> >>>>>> pipeline")
> >>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>>>>> ---
> >>>>>>  drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> >>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> >>>>>> b/drivers/media/platform/vsp1/vsp1_drm.c
> >>>>>> index f6d2f47a4058..06f74d410973 100644
> >>>>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> >>>>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> >>>>>> @@ -462,9 +462,9 @@ static int vsp1_du_pipeline_setup_inputs(struct
> >>>>>> vsp1_device *vsp1,
> >>>>>
> >>>>>
> >>>>> This looks like it complicates these conditionals more than we
> >>>>> perhaps need to.
> >>>>>
> >>>>> What do you think about adding something above the block comment here?:
> >>>>
> >>>> It is much better.
> >>>>
> >>>> This patch is accepted in media tree[1]. So not sure, should I send a
> >>>> follow up patch as optimization or drop this patch and send new one.
> >>>
> >>> Oh, I didn't realise these were in already. Sorry, I didn't see any review
> >>> on the list, and it was the earliest I had got to them.
> >>>
> >>>> Please suggest.
> >>>
> >>> Up to you, I don't think this would get dropped now it's integrated.
> >>> It's in, so if you want to update on top I believe that's fine.
> >>
> >> OK, Will send follow up patch as optimization.
> > 
> > That would be nice.
> > 
> > I don't think this patch should have been fast-tracked as a fix, as
> > RZ/G2L isn't supported in mainline yet as far as I can tell.
> > 
> > Hans, next time, could we get a notification instead of a silent merge ?
> 
> My apologies, it seemed a trivial fix, but I should have checked with you.
> 
> I jumped the gun here :-(

No worries, it can happen :-)

> >>>>> 	if (!drm_pipe->uif)
> >>>>> 		return 0;
> >>>>>
> >>>>>
> >>>>>>  	 * make sure it is present in the pipeline's list of entities if it
> >>>>>>  	 * wasn't already.
> >>>>>>  	 */
> >>>>>> -	if (!use_uif) {
> >>>>>> +	if (drm_pipe->uif && !use_uif) {
> >>>>>>  		drm_pipe->uif->pipe = NULL;
> >>>>>> -	} else if (!drm_pipe->uif->pipe) {
> >>>>>> +	} else if (drm_pipe->uif && !drm_pipe->uif->pipe) {>
> >>>>> 	drm_pipe->uif->pipe = pipe;
> >>>>>>  		list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);
> >>>>>>  	}
> >>>>>>

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access

[Biju Das]

RZ/G2L SoC has only BRS. This patch fixes null pointer access,when only
BRS is enabled.

Fixes: cbb7fa49c7466("media: v4l: vsp1: Rename BRU to BRx")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 86d5e3f4b1ff..f6d2f47a4058 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -245,7 +245,7 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
 		brx = &vsp1->bru->entity;
 	else if (pipe->brx && !drm_pipe->force_brx_release)
 		brx = pipe->brx;
-	else if (!vsp1->bru->entity.pipe)
+	else if (vsp1_feature(vsp1, VSP1_HAS_BRU) && !vsp1->bru->entity.pipe)
 		brx = &vsp1->bru->entity;
 	else
 		brx = &vsp1->brs->entity;
-- 
2.17.1