| Message ID | 20210301120828.6945-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VSP trivial enhancements | expand |
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>
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>
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>
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
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>
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;
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(-)