[PATCH V2] drm/bridge: adv7533: Fix adv7533_mode_valid for adv7533 and adv7535

[PATCH V2] drm/bridge: adv7533: Fix adv7533_mode_valid for adv7533 and adv7535

[Adam Ford]

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

Re: [PATCH V2] drm/bridge: adv7533: Fix adv7533_mode_valid for adv7533 and adv7535

[Robert Foss]

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.

Re: [PATCH V2] drm/bridge: adv7533: Fix adv7533_mode_valid for adv7533 and adv7535

[Yongqin Liu]

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

Re: [PATCH V2] drm/bridge: adv7533: Fix adv7533_mode_valid for adv7533 and adv7535

[Adam Ford]

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