[PATCH 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll

[PATCH 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll

[Adam Ford]

The P divider should be set based on the min and max values of
the fin pll which may vary between different platforms.
These ranges are defined per platform, but hard-coded values
were used instead which resulted in a smaller range available
on the i.MX8M[MNP] than what was possible.

Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index be5914caa17d..239d253a7d71 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -573,8 +573,8 @@ static unsigned long samsung_dsim_pll_find_pms(struct samsung_dsim *dsi,
 	u16 _m, best_m;
 	u8 _s, best_s;
 
-	p_min = DIV_ROUND_UP(fin, (12 * MHZ));
-	p_max = fin / (6 * MHZ);
+	p_min = DIV_ROUND_UP(fin, (driver_data->pll_fin_max * MHZ));
+	p_max = fin / (driver_data->pll_fin_min * MHZ);
 
 	for (_p = p_min; _p <= p_max; ++_p) {
 		for (_s = 0; _s <= 5; ++_s) {
-- 
2.40.1

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

[Adam Ford]

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 <frieder.schrempf@kontron.de> # Kontron BL i.MX8MM with HDMI monitor
Signed-off-by: Adam Ford <aford173@gmail.com>

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

Re: [PATCH 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll

[Frieder Schrempf]

Hi Adam,

On 12.12.23 04:32, Adam Ford wrote:
> The P divider should be set based on the min and max values of
> the fin pll which may vary between different platforms.
> These ranges are defined per platform, but hard-coded values
> were used instead which resulted in a smaller range available
> on the i.MX8M[MNP] than what was possible.
> 
> Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index be5914caa17d..239d253a7d71 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -573,8 +573,8 @@ static unsigned long samsung_dsim_pll_find_pms(struct samsung_dsim *dsi,
>  	u16 _m, best_m;
>  	u8 _s, best_s;
>  
> -	p_min = DIV_ROUND_UP(fin, (12 * MHZ));
> -	p_max = fin / (6 * MHZ);
> +	p_min = DIV_ROUND_UP(fin, (driver_data->pll_fin_max * MHZ));
> +	p_max = fin / (driver_data->pll_fin_min * MHZ);

I did some tinkering with the PLL settings the other day and this is
literally one of the things I came up with.

So I'm happy to provide:

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Regarding the P divider, I'm also wondering if there is an upper limit
for the p-value (not for the resulting fin_pll) that we should take into
account, too. The problem is that we have conflicts in the documentation
(again) so we don't really know what the correct limit would be.

There are the following ranges given in the RMs:

* 1..63 (i.MX8MM RM 13.7.8.18.4)
* 1..33 (i.MX8MM RM 13.7.10.1)
* 1..63 (i.MX8MP RM 13.2.3.1.5.2)
* 1..63 (i.MX8MP RM 13.7.2.4)

Unfortunately there are similar discrepancies for the other parameters
and limits.

Thanks
Frieder

>  
>  	for (_p = p_min; _p <= p_max; ++_p) {
>  		for (_s = 0; _s <= 5; ++_s) {

Re: [PATCH 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll

[Adam Ford]

On Tue, Dec 12, 2023 at 2:25 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Adam,
>
> On 12.12.23 04:32, Adam Ford wrote:
> > The P divider should be set based on the min and max values of
> > the fin pll which may vary between different platforms.
> > These ranges are defined per platform, but hard-coded values
> > were used instead which resulted in a smaller range available
> > on the i.MX8M[MNP] than what was possible.
> >
> > Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index be5914caa17d..239d253a7d71 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -573,8 +573,8 @@ static unsigned long samsung_dsim_pll_find_pms(struct samsung_dsim *dsi,
> >       u16 _m, best_m;
> >       u8 _s, best_s;
> >
> > -     p_min = DIV_ROUND_UP(fin, (12 * MHZ));
> > -     p_max = fin / (6 * MHZ);
> > +     p_min = DIV_ROUND_UP(fin, (driver_data->pll_fin_max * MHZ));
> > +     p_max = fin / (driver_data->pll_fin_min * MHZ);
>
> I did some tinkering with the PLL settings the other day and this is
> literally one of the things I came up with.
>
> So I'm happy to provide:
>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>

Thank you!

> Regarding the P divider, I'm also wondering if there is an upper limit
> for the p-value (not for the resulting fin_pll) that we should take into
> account, too. The problem is that we have conflicts in the documentation
> (again) so we don't really know what the correct limit would be.
>
> There are the following ranges given in the RMs:
>
> * 1..63 (i.MX8MM RM 13.7.8.18.4)
> * 1..33 (i.MX8MM RM 13.7.10.1)
> * 1..63 (i.MX8MP RM 13.2.3.1.5.2)
> * 1..63 (i.MX8MP RM 13.7.2.4)

1...63 (i.IMX8MN RM 13.7.2.4)
>
> Unfortunately there are similar discrepancies for the other parameters
> and limits.

For what it's worth, I compared these values to the NXP downstream
branch [1], and they seemed to indicate the values were as follows:

.p = { .min = 1, .max = 63, },
.m = { .min = 64, .max = 1023, },
.s = { .min = 0, .max = 5, },
.k = { .min = 0, .max = 32768, }, /* abs(k) */
.fin = { .min = 6000, .max = 300000, }, /* in KHz */
.fpref = { .min = 2000, .max = 30000, }, /* in KHz */
.fvco = { .min = 1050000, .max = 2100000, }, /* in KHz */

In a previous commit [2], I mentioned the fact that I reached out to
NXP asking about the discrepancies and my NXP Rep and I received the
response:

"Yes it is definitely wrong, the one that is part of the NOTE in
MIPI_DPHY_M_PLLPMS register table against PMS_P, PMS_M and PMS_S is
not correct. I will report this to Doc team, the one customer should
be take into account is the Table 13-40 DPHY PLL Parameters and the
Note above."

adam

[1] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/imx/sec_mipi_pll_1432x.h#L38C1-L47C1
[2] - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/bridge/samsung-dsim.c?h=next-20231212&id=54f1a83c72250b182fa7722b0c5f6eb5e769598d

>
> Thanks
> Frieder
>
> >
> >       for (_p = p_min; _p <= p_max; ++_p) {
> >               for (_s = 0; _s <= 5; ++_s) {
>

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

[Adam Ford]

On Mon, Dec 11, 2023 at 9:33 PM Adam Ford <aford173@gmail.com> 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 <frieder.schrempf@kontron.de> # Kontron BL i.MX8MM with HDMI monitor
> Signed-off-by: Adam Ford <aford173@gmail.com>
>
> 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
>

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

[Adam Ford]

On Mon, Dec 11, 2023 at 9:33 PM Adam Ford <aford173@gmail.com> 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 <frieder.schrempf@kontron.de> # Kontron BL i.MX8MM with HDMI monitor
> Signed-off-by: Adam Ford <aford173@gmail.com>

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
>

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

[Frieder Schrempf]

On 25.01.24 19:44, Adam Ford wrote:
> On Mon, Dec 11, 2023 at 9:33 PM Adam Ford <aford173@gmail.com> 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 <frieder.schrempf@kontron.de> # Kontron BL i.MX8MM with HDMI monitor
>> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> 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.

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

[Adam Ford]

On Mon, Jan 29, 2024 at 2:17 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> On 25.01.24 19:44, Adam Ford wrote:
> > On Mon, Dec 11, 2023 at 9:33 PM Adam Ford <aford173@gmail.com> 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 <frieder.schrempf@kontron.de> # Kontron BL i.MX8MM with HDMI monitor
> >> Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > 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.