| Message ID | 20241105111228.112813-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: adv7511: ADV7535 fixes | expand |
Hi Biju, Thank you for the patch. On Tue, Nov 05, 2024 at 11:12:19AM +0000, Biju Das wrote: > Fix out-of-bounds array in adv7511_dsi_config_timing_gen(), > when dsi lanes = 1. Does the hardware support using the internal timing generator with a single lane ? If so adv7511_dsi_config_timing_gen() should be fixed, otherwise that should be explained in the commit message, and mentioned with a comment in adv7533_parse_dt(). I would also print an error message in that case. If the internal timing generator can't be used with a single lane, the DT bindings should also be updated to document that. > Fixes: 78fa479d703c ("drm/bridge: adv7533: Use internal timing generator") > Reported-by: Hien Huynh <hien.huynh.px@renesas.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > Changes in v2: > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > - Dropped Archit Taneja invalid Mail address > --- > drivers/gpu/drm/bridge/adv7511/adv7533.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c > index 3e57ba838e5e..0c2236e53af5 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > @@ -185,6 +185,9 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > adv->use_timing_gen = !of_property_read_bool(np, > "adi,disable-timing-generator"); > > + if (adv->use_timing_gen && num_lanes == 1) > + return -EINVAL; > + > /* TODO: Check if these need to be parsed by DT or not */ > adv->rgb = true; > adv->embedded_sync = false;
Hi Laurent Pinchart, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 05 November 2024 16:06 > Subject: Re: [PATCH v2 2/2] drm: adv7511: Fix out-of-bounds array in clock_div_by_lanes > > Hi Biju, > > Thank you for the patch. > > On Tue, Nov 05, 2024 at 11:12:19AM +0000, Biju Das wrote: > > Fix out-of-bounds array in adv7511_dsi_config_timing_gen(), when dsi > > lanes = 1. > > Does the hardware support using the internal timing generator with a single lane ? If so As per the binding documentation [1], ADV7535 supports single lane. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml?h=next-20241106 > adv7511_dsi_config_timing_gen() should be fixed, otherwise that should be explained in the commit On RZ/G2L SMARC EVK platform, lanes=2,3,4 works ok, But with setting lanes=1, video is unstable by trying with clock_divider as 6,7 and 8 by updating the array and also disabling internal timing generator. > message, and mentioned with a comment in adv7533_parse_dt(). I would also print an error message in > that case. OK, this can be done. > > If the internal timing generator can't be used with a single lane, the DT bindings should also be > updated to document that. Even single lane with or without internal timing generator does not work on RZ/G2L. So, any users of ADV 7535 tested single lane?? Cheers, Biju > > > Fixes: 78fa479d703c ("drm/bridge: adv7533: Use internal timing > > generator") > > Reported-by: Hien Huynh <hien.huynh.px@renesas.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > Changes in v2: > > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > > - Dropped Archit Taneja invalid Mail address > > --- > > drivers/gpu/drm/bridge/adv7511/adv7533.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > index 3e57ba838e5e..0c2236e53af5 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > @@ -185,6 +185,9 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > > adv->use_timing_gen = !of_property_read_bool(np, > > "adi,disable-timing-generator"); > > > > + if (adv->use_timing_gen && num_lanes == 1) > > + return -EINVAL; > > + > > /* TODO: Check if these need to be parsed by DT or not */ > > adv->rgb = true; > > adv->embedded_sync = false; > > -- > Regards, > > Laurent Pinchart
On Wed, Nov 06, 2024 at 10:20:43AM +0000, Biju Das wrote: > Hi Laurent Pinchart, > > Thanks for the feedback. > > > -----Original Message----- > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Sent: 05 November 2024 16:06 > > Subject: Re: [PATCH v2 2/2] drm: adv7511: Fix out-of-bounds array in clock_div_by_lanes > > > > Hi Biju, > > > > Thank you for the patch. > > > > On Tue, Nov 05, 2024 at 11:12:19AM +0000, Biju Das wrote: > > > Fix out-of-bounds array in adv7511_dsi_config_timing_gen(), when dsi > > > lanes = 1. > > > > Does the hardware support using the internal timing generator with a single lane ? If so > > As per the binding documentation [1], ADV7535 supports single lane. > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml?h=next-20241106 > > > adv7511_dsi_config_timing_gen() should be fixed, otherwise that should be explained in the commit > > On RZ/G2L SMARC EVK platform, lanes=2,3,4 works ok, But with setting lanes=1, video is unstable > by trying with clock_divider as 6,7 and 8 by updating the array and also disabling internal timing generator. Is that an issue specific to that board, or to the chip in general ? If it's specific to the board, disabling 1 lane usage for everybody in the driver isn't the right option. > > message, and mentioned with a comment in adv7533_parse_dt(). I would also print an error message in > > that case. > > OK, this can be done. > > > If the internal timing generator can't be used with a single lane, the DT bindings should also be > > updated to document that. > > Even single lane with or without internal timing generator does not work on RZ/G2L. > > So, any users of ADV 7535 tested single lane?? > > > > Fixes: 78fa479d703c ("drm/bridge: adv7533: Use internal timing > > > generator") > > > Reported-by: Hien Huynh <hien.huynh.px@renesas.com> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > Changes in v2: > > > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > > > - Dropped Archit Taneja invalid Mail address > > > --- > > > drivers/gpu/drm/bridge/adv7511/adv7533.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > > b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > > index 3e57ba838e5e..0c2236e53af5 100644 > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > > @@ -185,6 +185,9 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) > > > adv->use_timing_gen = !of_property_read_bool(np, > > > "adi,disable-timing-generator"); > > > > > > + if (adv->use_timing_gen && num_lanes == 1) > > > + return -EINVAL; > > > + > > > /* TODO: Check if these need to be parsed by DT or not */ > > > adv->rgb = true; > > > adv->embedded_sync = false;
Hi Laurent Pinchart, > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Laurent Pinchart > Sent: 06 November 2024 13:18 > Subject: Re: [PATCH v2 2/2] drm: adv7511: Fix out-of-bounds array in clock_div_by_lanes > > On Wed, Nov 06, 2024 at 10:20:43AM +0000, Biju Das wrote: > > Hi Laurent Pinchart, > > > > Thanks for the feedback. > > > > > -----Original Message----- > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Sent: 05 November 2024 16:06 > > > Subject: Re: [PATCH v2 2/2] drm: adv7511: Fix out-of-bounds array in > > > clock_div_by_lanes > > > > > > Hi Biju, > > > > > > Thank you for the patch. > > > > > > On Tue, Nov 05, 2024 at 11:12:19AM +0000, Biju Das wrote: > > > > Fix out-of-bounds array in adv7511_dsi_config_timing_gen(), when > > > > dsi lanes = 1. > > > > > > Does the hardware support using the internal timing generator with a > > > single lane ? If so > > > > As per the binding documentation [1], ADV7535 supports single lane. > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tr > > ee/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml?h > > =next-20241106 > > > > > adv7511_dsi_config_timing_gen() should be fixed, otherwise that > > > should be explained in the commit > > > > On RZ/G2L SMARC EVK platform, lanes=2,3,4 works ok, But with setting > > lanes=1, video is unstable by trying with clock_divider as 6,7 and 8 by updating the array and also > disabling internal timing generator. > > Is that an issue specific to that board, or to the chip in general ? If it's specific to the board, > disabling 1 lane usage for everybody in the driver isn't the right option. At this moment, I do not know it is specific to board as with the current code with lane=1 and internal timing generator, it will lead to kernel crash. So looks like no one tested lane=1 with internal timing generator. Then the question is any user has tested lanes=1 by disabling internal generator?? Lane=1 corresponds to resolution 800x600@60 and below. Cheers, Biju
Hi Laurent Pinchart, > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 05 November 2024 16:06 > Subject: Re: [PATCH v2 2/2] drm: adv7511: Fix out-of-bounds array in clock_div_by_lanes > > Hi Biju, > > Thank you for the patch. > > On Tue, Nov 05, 2024 at 11:12:19AM +0000, Biju Das wrote: > > Fix out-of-bounds array in adv7511_dsi_config_timing_gen(), when dsi > > lanes = 1. > > Does the hardware support using the internal timing generator with a single lane ? If so > adv7511_dsi_config_timing_gen() should be fixed, otherwise that should be explained in the commit > message, and mentioned with a comment in adv7533_parse_dt(). I would also print an error message in > that case. > > If the internal timing generator can't be used with a single lane, the DT bindings should also be > updated to document that. As per [1], lanes = 1 is not supported in ADV7535/ADV7533. I will update the code and binding to remove lanes=1 support. [1] https://www.analog.com/media/en/technical-documentation/data-sheets/ADV7535.pdf Cheers, Biju > > > Fixes: 78fa479d703c ("drm/bridge: adv7533: Use internal timing > > generator") > > Reported-by: Hien Huynh <hien.huynh.px@renesas.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > Changes in v2: > > - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. > > - Dropped Archit Taneja invalid Mail address > > --- > > drivers/gpu/drm/bridge/adv7511/adv7533.c | 3 +++ > > 1 file changed, 3 insertions(+) > >
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c index 3e57ba838e5e..0c2236e53af5 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c @@ -185,6 +185,9 @@ int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv) adv->use_timing_gen = !of_property_read_bool(np, "adi,disable-timing-generator"); + if (adv->use_timing_gen && num_lanes == 1) + return -EINVAL; + /* TODO: Check if these need to be parsed by DT or not */ adv->rgb = true; adv->embedded_sync = false;
Fix out-of-bounds array in adv7511_dsi_config_timing_gen(), when dsi lanes = 1. Fixes: 78fa479d703c ("drm/bridge: adv7533: Use internal timing generator") Reported-by: Hien Huynh <hien.huynh.px@renesas.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- Changes in v2: - Added the tag "Cc: stable@vger.kernel.org" in the sign-off area. - Dropped Archit Taneja invalid Mail address --- drivers/gpu/drm/bridge/adv7511/adv7533.c | 3 +++ 1 file changed, 3 insertions(+)