[2/2] drm/bridge: samsung-dsim: Fix porch calcalcuation rounding

Commit Message

Adam Ford Dec. 12, 2023, 3:32 a.m. UTC

When using video sync pulses, the HFP, HBP, and HSA are divided between
the available lanes if there is more than one lane.  For certain
timings and lane configurations, the HFP may not be evenly divisible.
If the HFP is rounded down, it ends up being too small which can cause
some monitors to not sync properly. In these instances, adjust htotal
and hsync to round the HFP up, and recalculate the htotal.

Tested-by: Frieder Schrempf <[e-mail]> # Kontron BL i.MX8MM with HDMI monitor
Signed-off-by: Adam Ford <[e-mail]>

Comments

Adam Ford Dec. 12, 2023, 10:29 a.m. UTC | #1

On Mon, Dec 11, 2023 at 9:33 PM Adam Ford <[e-mail]> wrote:
>
> When using video sync pulses, the HFP, HBP, and HSA are divided between
> the available lanes if there is more than one lane.  For certain
> timings and lane configurations, the HFP may not be evenly divisible.
> If the HFP is rounded down, it ends up being too small which can cause
> some monitors to not sync properly. In these instances, adjust htotal
> and hsync to round the HFP up, and recalculate the htotal.
>

For anyone who's following this,  I added a note which I apparently
forgot to save:

This adds support for 720p60 in the i.MX8M Plus.

NXP uses a look-up table in their downstream code to accomplish this.
Using this calculation, the driver can adjust without the need for a
complicated table and should be flexible for different timings and
resolutions depending on the monitor.

I don't have a DSI analyzer, and this appears to only work on
i.MX8M Plus but not Mini and Nano for some reason, despite their
having a similar DSI bridge.

When Frieder teste this, he reported no changes on the Kontrol BL
i.MX8MM:   "So at least there is no negative impact in this case"


If someone else has an i.MX8MP, I would appreciate any feedback.

thanks

adam

> Tested-by: Frieder Schrempf <[e-mail]> # Kontron BL i.MX8MM with HDMI monitor
> Signed-off-by: Adam Ford <[e-mail]>
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 239d253a7d71..f5795da1d8bb 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1628,6 +1628,27 @@ static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
>                 adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>         }
>
> +       /*
> +        * When using video sync pulses, the HFP, HBP, and HSA are divided between
> +        * the available lanes if there is more than one lane.  For certain
> +        * timings and lane configurations, the HFP may not be evenly divisible.
> +        * If the HFP is rounded down, it ends up being too small which can cause
> +        * some monitors to not sync properly. In these instances, adjust htotal
> +        * and hsync to round the HFP up, and recalculate the htotal. Through trial
> +        * and error, it appears that the HBP and HSA do not appearto need the same
> +        * correction that HFP does.
> +        */
> +       if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE && dsi->lanes > 1) {
> +               int hfp = adjusted_mode->hsync_start - adjusted_mode->hdisplay;
> +               int remainder = hfp % dsi->lanes;
> +
> +               if (remainder) {
> +                       adjusted_mode->hsync_start += remainder;
> +                       adjusted_mode->hsync_end   += remainder;
> +                       adjusted_mode->htotal      += remainder;
> +               }
> +       }
> +
>         return 0;
>  }
>
> --
> 2.40.1
>

Adam Ford Jan. 25, 2024, 6:44 p.m. UTC | #2

On Mon, Dec 11, 2023 at 9:33 PM Adam Ford <[e-mail]> wrote:
>
> When using video sync pulses, the HFP, HBP, and HSA are divided between
> the available lanes if there is more than one lane.  For certain
> timings and lane configurations, the HFP may not be evenly divisible.
> If the HFP is rounded down, it ends up being too small which can cause
> some monitors to not sync properly. In these instances, adjust htotal
> and hsync to round the HFP up, and recalculate the htotal.
>
> Tested-by: Frieder Schrempf <[e-mail]> # Kontron BL i.MX8MM with HDMI monitor
> Signed-off-by: Adam Ford <[e-mail]>

Gentle nudge on this one.  Basically this fixes an issue with the 8MP,
but it's still unknown why it doesn't work on 8MM or 8MN, but Frieder
confirmed there are no regressions on 8MM or 8MN.

adam


>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 239d253a7d71..f5795da1d8bb 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1628,6 +1628,27 @@ static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
>                 adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>         }
>
> +       /*
> +        * When using video sync pulses, the HFP, HBP, and HSA are divided between
> +        * the available lanes if there is more than one lane.  For certain
> +        * timings and lane configurations, the HFP may not be evenly divisible.
> +        * If the HFP is rounded down, it ends up being too small which can cause
> +        * some monitors to not sync properly. In these instances, adjust htotal
> +        * and hsync to round the HFP up, and recalculate the htotal. Through trial
> +        * and error, it appears that the HBP and HSA do not appearto need the same
> +        * correction that HFP does.
> +        */
> +       if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE && dsi->lanes > 1) {
> +               int hfp = adjusted_mode->hsync_start - adjusted_mode->hdisplay;
> +               int remainder = hfp % dsi->lanes;
> +
> +               if (remainder) {
> +                       adjusted_mode->hsync_start += remainder;
> +                       adjusted_mode->hsync_end   += remainder;
> +                       adjusted_mode->htotal      += remainder;
> +               }
> +       }
> +
>         return 0;
>  }
>
> --
> 2.40.1
>

Frieder Schrempf Jan. 29, 2024, 8:17 a.m. UTC | #3

On 25.01.24 19:44, Adam Ford wrote:
> On Mon, Dec 11, 2023 at 9:33 PM Adam Ford <[e-mail]> wrote:
>>
>> When using video sync pulses, the HFP, HBP, and HSA are divided between
>> the available lanes if there is more than one lane.  For certain
>> timings and lane configurations, the HFP may not be evenly divisible.
>> If the HFP is rounded down, it ends up being too small which can cause
>> some monitors to not sync properly. In these instances, adjust htotal
>> and hsync to round the HFP up, and recalculate the htotal.
>>
>> Tested-by: Frieder Schrempf <[e-mail]> # Kontron BL i.MX8MM with HDMI monitor
>> Signed-off-by: Adam Ford <[e-mail]>
> 
> Gentle nudge on this one.  Basically this fixes an issue with the 8MP,
> but it's still unknown why it doesn't work on 8MM or 8MN, but Frieder
> confirmed there are no regressions on 8MM or 8MN.

I only tested two specific display setups on i.MX8MM. So of course I
can't confirm the absence of regressions in general.

Anyway, I think this should be applied.

Adam Ford Feb. 8, 2024, 1:15 p.m. UTC | #4

On Mon, Jan 29, 2024 at 2:17 AM Frieder Schrempf
<[e-mail]> wrote:
>
> On 25.01.24 19:44, Adam Ford wrote:
> > On Mon, Dec 11, 2023 at 9:33 PM Adam Ford <[e-mail]> wrote:
> >>
> >> When using video sync pulses, the HFP, HBP, and HSA are divided between
> >> the available lanes if there is more than one lane.  For certain
> >> timings and lane configurations, the HFP may not be evenly divisible.
> >> If the HFP is rounded down, it ends up being too small which can cause
> >> some monitors to not sync properly. In these instances, adjust htotal
> >> and hsync to round the HFP up, and recalculate the htotal.
> >>
> >> Tested-by: Frieder Schrempf <[e-mail]> # Kontron BL i.MX8MM with HDMI monitor
> >> Signed-off-by: Adam Ford <[e-mail]>
> >
> > Gentle nudge on this one.  Basically this fixes an issue with the 8MP,
> > but it's still unknown why it doesn't work on 8MM or 8MN, but Frieder
> > confirmed there are no regressions on 8MM or 8MN.
>

Inki,

Is there something you need which is holding this back?  It's been
nearly two months since I posted the initial patch.

Thank you,

adam

> I only tested two specific display setups on i.MX8MM. So of course I
> can't confirm the absence of regressions in general.
>
> Anyway, I think this should be applied.

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 239d253a7d71..f5795da1d8bb 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1628,6 +1628,27 @@  static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
 		adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	}
 
+	/*
+	 * When using video sync pulses, the HFP, HBP, and HSA are divided between
+	 * the available lanes if there is more than one lane.  For certain
+	 * timings and lane configurations, the HFP may not be evenly divisible.
+	 * If the HFP is rounded down, it ends up being too small which can cause
+	 * some monitors to not sync properly. In these instances, adjust htotal
+	 * and hsync to round the HFP up, and recalculate the htotal. Through trial
+	 * and error, it appears that the HBP and HSA do not appearto need the same
+	 * correction that HFP does.
+	 */
+	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE && dsi->lanes > 1) {
+		int hfp = adjusted_mode->hsync_start - adjusted_mode->hdisplay;
+		int remainder = hfp % dsi->lanes;
+
+		if (remainder) {
+			adjusted_mode->hsync_start += remainder;
+			adjusted_mode->hsync_end   += remainder;
+			adjusted_mode->htotal      += remainder;
+		}
+	}
+
 	return 0;
 }