| Message ID | 20230319125524.58803-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] drm/bridge: adv7533: Fix adv7533_mode_valid for adv7533 and adv7535 | expand |
On Sun, Mar 19, 2023 at 1:55 PM Adam Ford <aford173@gmail.com> wrote: > > When dynamically switching lanes was removed, the intent of the code > was to check to make sure that higher speed items used 4 lanes, but > it had the unintended consequence of removing the slower speeds for > 4-lane users. > > This attempts to remedy this by doing a check to see that the > max frequency doesn't exceed the chip limit, and a second > check to make sure that the max bit-rate doesn't exceed the > number of lanes * max bit rate / lane. > > Fixes: 9a0cdcd6649b ("drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge") > Reviewed-by: Robert Foss <rfoss@kernel.org> > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > > V2: Fix whitespace in comment > Remove TODO comment > Add R-B from Robert. > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c > index fdfeadcefe80..7e3e56441aed 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > @@ -103,22 +103,19 @@ void adv7533_dsi_power_off(struct adv7511 *adv) > enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv, > const struct drm_display_mode *mode) > { > - int lanes; > + unsigned long max_lane_freq; > struct mipi_dsi_device *dsi = adv->dsi; > + u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > > - if (mode->clock > 80000) > - lanes = 4; > - else > - lanes = 3; > - > - /* > - * TODO: add support for dynamic switching of lanes > - * by using the bridge pre_enable() op . Till then filter > - * out the modes which shall need different number of lanes > - * than what was configured in the device tree. > - */ > - if (lanes != dsi->lanes) > - return MODE_BAD; > + /* Check max clock for either 7533 or 7535 */ > + if (mode->clock > (adv->type == ADV7533 ? 80000 : 148500)) > + return MODE_CLOCK_HIGH; > + > + /* Check max clock for each lane */ > + max_lane_freq = (adv->type == ADV7533 ? 800000 : 891000); > + > + if (mode->clock * bpp > max_lane_freq * adv->num_dsi_lanes) > + return MODE_CLOCK_HIGH; > > return MODE_OK; > } > -- > 2.34.1 > Applied, thanks! Repo: https://cgit.freedesktop.org/drm/drm-misc/ Rob.
Hi, Adam, All On Sun, 19 Mar 2023 at 20:55, Adam Ford <aford173@gmail.com> wrote: > > When dynamically switching lanes was removed, the intent of the code > was to check to make sure that higher speed items used 4 lanes, but > it had the unintended consequence of removing the slower speeds for > 4-lane users. > > This attempts to remedy this by doing a check to see that the > max frequency doesn't exceed the chip limit, and a second > check to make sure that the max bit-rate doesn't exceed the > number of lanes * max bit rate / lane. > > Fixes: 9a0cdcd6649b ("drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge") > Reviewed-by: Robert Foss <rfoss@kernel.org> > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > > V2: Fix whitespace in comment > Remove TODO comment > Add R-B from Robert. With this change, the ACK android-mainline based hikey960 build failed to show UI on the HDMI monitor connected, but it works if I revert this change. Here is the serial console output: http://ix.io/4zK8 Not sure if you have any idea what the problem is there, and how to have it fixed. Please let me know if you need any other information. Thanks, Yongqin Liu > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c > index fdfeadcefe80..7e3e56441aed 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > @@ -103,22 +103,19 @@ void adv7533_dsi_power_off(struct adv7511 *adv) > enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv, > const struct drm_display_mode *mode) > { > - int lanes; > + unsigned long max_lane_freq; > struct mipi_dsi_device *dsi = adv->dsi; > + u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > > - if (mode->clock > 80000) > - lanes = 4; > - else > - lanes = 3; > - > - /* > - * TODO: add support for dynamic switching of lanes > - * by using the bridge pre_enable() op . Till then filter > - * out the modes which shall need different number of lanes > - * than what was configured in the device tree. > - */ > - if (lanes != dsi->lanes) > - return MODE_BAD; > + /* Check max clock for either 7533 or 7535 */ > + if (mode->clock > (adv->type == ADV7533 ? 80000 : 148500)) > + return MODE_CLOCK_HIGH; > + > + /* Check max clock for each lane */ > + max_lane_freq = (adv->type == ADV7533 ? 800000 : 891000); > + > + if (mode->clock * bpp > max_lane_freq * adv->num_dsi_lanes) > + return MODE_CLOCK_HIGH; > > return MODE_OK; > } > -- > 2.34.1 > -- Best Regards, Yongqin Liu --------------------------------------------------------------- #mailing list linaro-android@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-android
On Tue, Jul 4, 2023 at 11:05 AM Yongqin Liu <yongqin.liu@linaro.org> wrote: > > Hi, Adam, All > > On Sun, 19 Mar 2023 at 20:55, Adam Ford <aford173@gmail.com> wrote: > > > > When dynamically switching lanes was removed, the intent of the code > > was to check to make sure that higher speed items used 4 lanes, but > > it had the unintended consequence of removing the slower speeds for > > 4-lane users. > > > > This attempts to remedy this by doing a check to see that the > > max frequency doesn't exceed the chip limit, and a second > > check to make sure that the max bit-rate doesn't exceed the > > number of lanes * max bit rate / lane. > > > > Fixes: 9a0cdcd6649b ("drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge") > > Reviewed-by: Robert Foss <rfoss@kernel.org> > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > > > V2: Fix whitespace in comment > > Remove TODO comment > > Add R-B from Robert. > > With this change, the ACK android-mainline based hikey960 build failed > to show UI on the HDMI > monitor connected, but it works if I revert this change. > Here is the serial console output: http://ix.io/4zK8 > > Not sure if you have any idea what the problem is there, > and how to have it fixed. > > Please let me know if you need any other information. I'd be curious to know the desired clock rate and the number of lanes you're trying to use. adam > > Thanks, > Yongqin Liu > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > index fdfeadcefe80..7e3e56441aed 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c > > @@ -103,22 +103,19 @@ void adv7533_dsi_power_off(struct adv7511 *adv) > > enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv, > > const struct drm_display_mode *mode) > > { > > - int lanes; > > + unsigned long max_lane_freq; > > struct mipi_dsi_device *dsi = adv->dsi; > > + u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > > > > - if (mode->clock > 80000) > > - lanes = 4; > > - else > > - lanes = 3; > > - > > - /* > > - * TODO: add support for dynamic switching of lanes > > - * by using the bridge pre_enable() op . Till then filter > > - * out the modes which shall need different number of lanes > > - * than what was configured in the device tree. > > - */ > > - if (lanes != dsi->lanes) > > - return MODE_BAD; > > + /* Check max clock for either 7533 or 7535 */ > > + if (mode->clock > (adv->type == ADV7533 ? 80000 : 148500)) > > + return MODE_CLOCK_HIGH; > > + > > + /* Check max clock for each lane */ > > + max_lane_freq = (adv->type == ADV7533 ? 800000 : 891000); > > + > > + if (mode->clock * bpp > max_lane_freq * adv->num_dsi_lanes) > > + return MODE_CLOCK_HIGH; > > > > return MODE_OK; > > } > > -- > > 2.34.1 > > > > > -- > Best Regards, > Yongqin Liu > --------------------------------------------------------------- > #mailing list > linaro-android@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-android
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c index fdfeadcefe80..7e3e56441aed 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c @@ -103,22 +103,19 @@ void adv7533_dsi_power_off(struct adv7511 *adv) enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv, const struct drm_display_mode *mode) { - int lanes; + unsigned long max_lane_freq; struct mipi_dsi_device *dsi = adv->dsi; + u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); - if (mode->clock > 80000) - lanes = 4; - else - lanes = 3; - - /* - * TODO: add support for dynamic switching of lanes - * by using the bridge pre_enable() op . Till then filter - * out the modes which shall need different number of lanes - * than what was configured in the device tree. - */ - if (lanes != dsi->lanes) - return MODE_BAD; + /* Check max clock for either 7533 or 7535 */ + if (mode->clock > (adv->type == ADV7533 ? 80000 : 148500)) + return MODE_CLOCK_HIGH; + + /* Check max clock for each lane */ + max_lane_freq = (adv->type == ADV7533 ? 800000 : 891000); + + if (mode->clock * bpp > max_lane_freq * adv->num_dsi_lanes) + return MODE_CLOCK_HIGH; return MODE_OK; }
When dynamically switching lanes was removed, the intent of the code was to check to make sure that higher speed items used 4 lanes, but it had the unintended consequence of removing the slower speeds for 4-lane users. This attempts to remedy this by doing a check to see that the max frequency doesn't exceed the chip limit, and a second check to make sure that the max bit-rate doesn't exceed the number of lanes * max bit rate / lane. Fixes: 9a0cdcd6649b ("drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge") Reviewed-by: Robert Foss <rfoss@kernel.org> Signed-off-by: Adam Ford <aford173@gmail.com> --- V2: Fix whitespace in comment Remove TODO comment Add R-B from Robert.