* [PATCH 0/2] VSP trivial enhancements @ 2021-03-01 12:08 Biju Das 2021-03-01 12:08 ` [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access Biju Das 2021-03-01 12:08 ` [PATCH 2/2] media: v4l: vsp1: Fix uif " Biju Das 0 siblings, 2 replies; 18+ messages in thread From: Biju Das @ 2021-03-01 12:08 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access 2021-03-01 12:08 [PATCH 0/2] VSP trivial enhancements Biju Das @ 2021-03-01 12:08 ` Biju Das 2021-03-08 23:06 ` Kieran Bingham 2021-03-01 12:08 ` [PATCH 2/2] media: v4l: vsp1: Fix uif " Biju Das 1 sibling, 1 reply; 18+ messages in thread From: Biju Das @ 2021-03-01 12:08 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access 2021-03-01 12:08 ` [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access Biju Das @ 2021-03-08 23:06 ` Kieran Bingham 2021-03-11 7:15 ` Biju Das 0 siblings, 1 reply; 18+ messages in thread From: Kieran Bingham @ 2021-03-08 23:06 UTC (permalink / raw) To: Biju Das, Mauro Carvalho Chehab Cc: Laurent Pinchart, Kieran Bingham, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access 2021-03-08 23:06 ` Kieran Bingham @ 2021-03-11 7:15 ` Biju Das 2021-03-15 3:36 ` Laurent Pinchart 0 siblings, 1 reply; 18+ messages in thread From: Biju Das @ 2021-03-11 7:15 UTC (permalink / raw) To: kieran.bingham+renesas, Mauro Carvalho Chehab Cc: Laurent Pinchart, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access 2021-03-11 7:15 ` Biju Das @ 2021-03-15 3:36 ` Laurent Pinchart 2021-03-15 9:15 ` Biju Das 0 siblings, 1 reply; 18+ messages in thread From: Laurent Pinchart @ 2021-03-15 3:36 UTC (permalink / raw) To: Biju Das Cc: kieran.bingham+renesas, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access 2021-03-15 3:36 ` Laurent Pinchart @ 2021-03-15 9:15 ` Biju Das 2021-03-15 9:27 ` Laurent Pinchart 0 siblings, 1 reply; 18+ messages in thread From: Biju Das @ 2021-03-15 9:15 UTC (permalink / raw) To: Laurent Pinchart Cc: kieran.bingham+renesas, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access 2021-03-15 9:15 ` Biju Das @ 2021-03-15 9:27 ` Laurent Pinchart 0 siblings, 0 replies; 18+ messages in thread From: Laurent Pinchart @ 2021-03-15 9:27 UTC (permalink / raw) To: Biju Das Cc: kieran.bingham+renesas, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-01 12:08 [PATCH 0/2] VSP trivial enhancements Biju Das 2021-03-01 12:08 ` [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access Biju Das @ 2021-03-01 12:08 ` Biju Das 2021-03-08 23:00 ` Kieran Bingham 1 sibling, 1 reply; 18+ messages in thread From: Biju Das @ 2021-03-01 12:08 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-01 12:08 ` [PATCH 2/2] media: v4l: vsp1: Fix uif " Biju Das @ 2021-03-08 23:00 ` Kieran Bingham 2021-03-10 13:56 ` Biju Das 0 siblings, 1 reply; 18+ messages in thread From: Kieran Bingham @ 2021-03-08 23:00 UTC (permalink / raw) To: Biju Das, Mauro Carvalho Chehab Cc: Laurent Pinchart, Kieran Bingham, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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); > } > ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-08 23:00 ` Kieran Bingham @ 2021-03-10 13:56 ` Biju Das 2021-03-10 14:39 ` Kieran Bingham 0 siblings, 1 reply; 18+ messages in thread From: Biju Das @ 2021-03-10 13:56 UTC (permalink / raw) To: kieran.bingham+renesas, Mauro Carvalho Chehab Cc: Laurent Pinchart, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad, Hans Verkuil 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); > > } > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-10 13:56 ` Biju Das @ 2021-03-10 14:39 ` Kieran Bingham 2021-03-10 14:50 ` Biju Das 0 siblings, 1 reply; 18+ messages in thread From: Kieran Bingham @ 2021-03-10 14:39 UTC (permalink / raw) To: Biju Das, Mauro Carvalho Chehab Cc: Laurent Pinchart, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad, Hans Verkuil 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); >>> } >>> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-10 14:39 ` Kieran Bingham @ 2021-03-10 14:50 ` Biju Das 2021-03-15 3:42 ` Laurent Pinchart 0 siblings, 1 reply; 18+ messages in thread From: Biju Das @ 2021-03-10 14:50 UTC (permalink / raw) To: kieran.bingham+renesas, Mauro Carvalho Chehab Cc: Laurent Pinchart, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad, Hans Verkuil 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&data=04%7C01%7Cbiju.das.jz%40bp.ren > > esas.com%7Cad7bafadc61345db66e908d8e3d24f3c%7C53d82571da1947e49cb4625a > > 166a4a2a%7C0%7C0%7C637509839726919053%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi > > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000& > > sdata=jb%2BzwAJD6%2FxBKsQEryhDeKnJbgI%2F3RNZCIXfYU4EBCw%3D&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); > >>> } > >>> > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-10 14:50 ` Biju Das @ 2021-03-15 3:42 ` Laurent Pinchart 2021-03-15 8:21 ` Biju Das 2021-03-16 8:21 ` Hans Verkuil 0 siblings, 2 replies; 18+ messages in thread From: Laurent Pinchart @ 2021-03-15 3:42 UTC (permalink / raw) To: Biju Das Cc: kieran.bingham+renesas, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad, Hans Verkuil 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-15 3:42 ` Laurent Pinchart @ 2021-03-15 8:21 ` Biju Das 2021-03-16 1:01 ` Laurent Pinchart 2021-03-16 8:21 ` Hans Verkuil 1 sibling, 1 reply; 18+ messages in thread From: Biju Das @ 2021-03-15 8:21 UTC (permalink / raw) To: Laurent Pinchart Cc: kieran.bingham+renesas, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad, Hans Verkuil 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-15 8:21 ` Biju Das @ 2021-03-16 1:01 ` Laurent Pinchart 0 siblings, 0 replies; 18+ messages in thread From: Laurent Pinchart @ 2021-03-16 1:01 UTC (permalink / raw) To: Biju Das Cc: kieran.bingham+renesas, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad, Hans Verkuil 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-15 3:42 ` Laurent Pinchart 2021-03-15 8:21 ` Biju Das @ 2021-03-16 8:21 ` Hans Verkuil 2021-03-16 21:37 ` Laurent Pinchart 1 sibling, 1 reply; 18+ messages in thread From: Hans Verkuil @ 2021-03-16 8:21 UTC (permalink / raw) To: Laurent Pinchart, Biju Das Cc: kieran.bingham+renesas, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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); >>>>>> } >>>>>> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: v4l: vsp1: Fix uif null pointer access 2021-03-16 8:21 ` Hans Verkuil @ 2021-03-16 21:37 ` Laurent Pinchart 0 siblings, 0 replies; 18+ messages in thread From: Laurent Pinchart @ 2021-03-16 21:37 UTC (permalink / raw) To: Hans Verkuil Cc: Biju Das, kieran.bingham+renesas, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 0/2] VSP trivial enhancements @ 2021-03-01 12:02 Biju Das 2021-03-01 12:02 ` [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access Biju Das 0 siblings, 1 reply; 18+ messages in thread From: Biju Das @ 2021-03-01 12:02 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access 2021-03-01 12:02 [PATCH 0/2] VSP trivial enhancements Biju Das @ 2021-03-01 12:02 ` Biju Das 0 siblings, 0 replies; 18+ messages in thread From: Biju Das @ 2021-03-01 12:02 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Biju Das, Laurent Pinchart, Kieran Bingham, linux-media, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das, Prabhakar Mahadev Lad 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-03-16 21:39 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-01 12:08 [PATCH 0/2] VSP trivial enhancements Biju Das 2021-03-01 12:08 ` [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access Biju Das 2021-03-08 23:06 ` Kieran Bingham 2021-03-11 7:15 ` Biju Das 2021-03-15 3:36 ` Laurent Pinchart 2021-03-15 9:15 ` Biju Das 2021-03-15 9:27 ` Laurent Pinchart 2021-03-01 12:08 ` [PATCH 2/2] media: v4l: vsp1: Fix uif " Biju Das 2021-03-08 23:00 ` Kieran Bingham 2021-03-10 13:56 ` Biju Das 2021-03-10 14:39 ` Kieran Bingham 2021-03-10 14:50 ` Biju Das 2021-03-15 3:42 ` Laurent Pinchart 2021-03-15 8:21 ` Biju Das 2021-03-16 1:01 ` Laurent Pinchart 2021-03-16 8:21 ` Hans Verkuil 2021-03-16 21:37 ` Laurent Pinchart -- strict thread matches above, loose matches on Subject: below -- 2021-03-01 12:02 [PATCH 0/2] VSP trivial enhancements Biju Das 2021-03-01 12:02 ` [PATCH 1/2] media: v4l: vsp1: Fix bru null pointer access Biju Das
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).