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.
As noted by Frieder, there are descripencies between the reference
manuals of the Mini, Nano and Plus, so I reached out to my NXP
rep and got the following response regarding the varing notes
in the documentation.
"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."
With this patch, the clock rates now match the values used in NXP's
downstream kernel.
Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
Signed-off-by: Adam Ford <[email protected]>
Reviewed-by: Frieder Schrempf <[email protected]>
Tested-by: Frieder Schrempf <[email protected]>
---
V2: Only update the commit message to reflect why these values
were chosen. No code change present
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 95fedc68b0ae..8476650c477c 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -574,8 +574,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.43.0
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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
Signed-off-by: Adam Ford <[email protected]>
---
V2: No changes
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 8476650c477c..52939211fe93 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1606,6 +1606,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.43.0
On Sun, Feb 11, 2024 at 5:09 PM Adam Ford <[email protected]> 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.
>
> As noted by Frieder, there are descripencies between the reference
> manuals of the Mini, Nano and Plus, so I reached out to my NXP
> rep and got the following response regarding the varing notes
> in the documentation.
>
> "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."
>
> With this patch, the clock rates now match the values used in NXP's
> downstream kernel.
>
Inki,
Any change either or both of these patches could get applied? It's
been several months since the first submission.
Fabio - Do you have an 8MP-evk that you could test to see if there is
any impact? Between these two patches, it fixes the 720p@60 and
likely others too.
adam
> Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> Signed-off-by: Adam Ford <[email protected]>
> Reviewed-by: Frieder Schrempf <[email protected]>
> Tested-by: Frieder Schrempf <[email protected]>
> ---
> V2: Only update the commit message to reflect why these values
> were chosen. No code change present
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 95fedc68b0ae..8476650c477c 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -574,8 +574,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.43.0
>
On Sun, Feb 11, 2024 at 5:09 PM Adam Ford <[email protected]> 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.
>
Marek V and Marek S,
Would either of you be willing to test that this doesn't break your
applications?
thanks
adam
> Tested-by: Frieder Schrempf <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
> Signed-off-by: Adam Ford <[email protected]>
> ---
> V2: No changes
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 8476650c477c..52939211fe93 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1606,6 +1606,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.43.0
>
On 2/12/24 12:09 AM, 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.
>
> As noted by Frieder, there are descripencies between the reference
> manuals of the Mini, Nano and Plus, so I reached out to my NXP
> rep and got the following response regarding the varing notes
> in the documentation.
>
> "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."
>
> With this patch, the clock rates now match the values used in NXP's
> downstream kernel.
>
> Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> Signed-off-by: Adam Ford <[email protected]>
> Reviewed-by: Frieder Schrempf <[email protected]>
> Tested-by: Frieder Schrempf <[email protected]>
> ---
> V2: Only update the commit message to reflect why these values
> were chosen. No code change present
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 95fedc68b0ae..8476650c477c 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -574,8 +574,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));
The parenthesis around driver_data... are not needed.
With that fixed:
Reviewed-by: Marek Vasut <[email protected]>
On 2/12/24 12:09 AM, Adam Ford 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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
> Signed-off-by: Adam Ford <[email protected]>
> ---
> V2: No changes
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 8476650c477c..52939211fe93 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1606,6 +1606,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) {
Does this also apply to mode with sync events (I suspect it does), so
the condition here should likely be if (!...burst mode) , right ?
On Sun, Apr 21, 2024 at 9:36 AM Marek Vasut <[email protected]> wrote:
>
> On 2/12/24 12:09 AM, Adam Ford 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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
> > Signed-off-by: Adam Ford <[email protected]>
> > ---
> > V2: No changes
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 8476650c477c..52939211fe93 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1606,6 +1606,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) {
>
> Does this also apply to mode with sync events (I suspect it does), so
> the condition here should likely be if (!...burst mode) , right ?
Thanks for the review!
I was only able to test it with the DSI->ADV6535 bridge, and I'll
admit I don't know a lot about DSI interface since I don't have a copy
of the spec to read.
Are you proposing this should be:
if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->lanes > 1) {
I just want to make sure I understand what you're requesting.
thanks
adam
On Mon, Apr 22, 2024 at 8:01 AM Marek Vasut <[email protected]> wrote:
>
> On 4/22/24 2:09 PM, Adam Ford wrote:
> > On Sun, Apr 21, 2024 at 9:36 AM Marek Vasut <[email protected]> wrote:
> >>
> >> On 2/12/24 12:09 AM, Adam Ford 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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
> >>> Signed-off-by: Adam Ford <[email protected]>
> >>> ---
> >>> V2: No changes
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> index 8476650c477c..52939211fe93 100644
> >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> @@ -1606,6 +1606,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) {
> >>
> >> Does this also apply to mode with sync events (I suspect it does), so
> >> the condition here should likely be if (!...burst mode) , right ?
> >
> > Thanks for the review!
> >
> > I was only able to test it with the DSI->ADV6535 bridge, and I'll
> > admit I don't know a lot about DSI interface since I don't have a copy
> > of the spec to read.
> >
> > Are you proposing this should be:
> >
> > if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->lanes > 1) {
> >
> > I just want to make sure I understand what you're requesting.
>
> Yes, exactly this.
Do you think it should also include checks for
MIPI_DSI_MODE_VIDEO_NO_HFP, MIPI_DSI_MODE_VIDEO_NO_HBP or
MIPI_DSI_MODE_VIDEO_NO_HSA?
It seems like if any of these are set, we should skip this rounding stuff.
adam
On 4/22/24 2:09 PM, Adam Ford wrote:
> On Sun, Apr 21, 2024 at 9:36 AM Marek Vasut <[email protected]> wrote:
>>
>> On 2/12/24 12:09 AM, Adam Ford 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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
>>> Signed-off-by: Adam Ford <[email protected]>
>>> ---
>>> V2: No changes
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index 8476650c477c..52939211fe93 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -1606,6 +1606,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) {
>>
>> Does this also apply to mode with sync events (I suspect it does), so
>> the condition here should likely be if (!...burst mode) , right ?
>
> Thanks for the review!
>
> I was only able to test it with the DSI->ADV6535 bridge, and I'll
> admit I don't know a lot about DSI interface since I don't have a copy
> of the spec to read.
>
> Are you proposing this should be:
>
> if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->lanes > 1) {
>
> I just want to make sure I understand what you're requesting.
Yes, exactly this.
On 4/22/24 3:04 PM, Adam Ford wrote:
> On Mon, Apr 22, 2024 at 8:01 AM Marek Vasut <[email protected]> wrote:
>>
>> On 4/22/24 2:09 PM, Adam Ford wrote:
>>> On Sun, Apr 21, 2024 at 9:36 AM Marek Vasut <[email protected]> wrote:
>>>>
>>>> On 2/12/24 12:09 AM, Adam Ford 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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
>>>>> Signed-off-by: Adam Ford <[email protected]>
>>>>> ---
>>>>> V2: No changes
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> index 8476650c477c..52939211fe93 100644
>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> @@ -1606,6 +1606,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) {
>>>>
>>>> Does this also apply to mode with sync events (I suspect it does), so
>>>> the condition here should likely be if (!...burst mode) , right ?
>>>
>>> Thanks for the review!
>>>
>>> I was only able to test it with the DSI->ADV6535 bridge, and I'll
>>> admit I don't know a lot about DSI interface since I don't have a copy
>>> of the spec to read.
>>>
>>> Are you proposing this should be:
>>>
>>> if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->lanes > 1) {
>>>
>>> I just want to make sure I understand what you're requesting.
>>
>> Yes, exactly this.
>
> Do you think it should also include checks for
> MIPI_DSI_MODE_VIDEO_NO_HFP, MIPI_DSI_MODE_VIDEO_NO_HBP or
> MIPI_DSI_MODE_VIDEO_NO_HSA?
>
> It seems like if any of these are set, we should skip this rounding stuff.
Now that you ask this question, shouldn't this actually apply
unconditionally , no matter which mode is in use ?
On 12.02.2024 00:09, 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.
>
> As noted by Frieder, there are descripencies between the reference
> manuals of the Mini, Nano and Plus, so I reached out to my NXP
> rep and got the following response regarding the varing notes
> in the documentation.
>
> "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."
>
> With this patch, the clock rates now match the values used in NXP's
> downstream kernel.
>
> Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> Signed-off-by: Adam Ford <[email protected]>
> Reviewed-by: Frieder Schrempf <[email protected]>
> Tested-by: Frieder Schrempf <[email protected]>
Feel free to add:
Tested-by: Marek Szyprowski <[email protected]>
although all Exynos based boards have panels with fixed timings afair.
> ---
> V2: Only update the commit message to reflect why these values
> were chosen. No code change present
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 95fedc68b0ae..8476650c477c 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -574,8 +574,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) {
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On 12.02.2024 00:09, Adam Ford 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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
> Signed-off-by: Adam Ford <[email protected]>
Tested-by: Marek Szyprowski <[email protected]>
> ---
> V2: No changes
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 8476650c477c..52939211fe93 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1606,6 +1606,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;
> }
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Thu, Apr 25, 2024 at 4:19 AM Marek Szyprowski
<[email protected]> wrote:
>
> On 12.02.2024 00:09, Adam Ford 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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
> > Signed-off-by: Adam Ford <[email protected]>
>
> Tested-by: Marek Szyprowski <[email protected]>
Thank you very much for testing!
>
> > ---
> > V2: No changes
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 8476650c477c..52939211fe93 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1606,6 +1606,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) {
Frieder & Marek S,
Marek V is proposing we eliminate the check against the flags and do
it unconditionally. If I send you both a different patch, would you
be willing to try them on your platforms? I don't want to risk
breaking a board.
I used the check above from the NXP downstream kernel, so it felt
safe, but I am not as familiar with the different DSI modes, so I am
not sure what the impact would be if this read:
if (dsi->lanes > 1) {
Does anyone else have an opinion on this?
> > + 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;
> > }
> >
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
On 25.04.2024 22:30, Adam Ford wrote:
> On Thu, Apr 25, 2024 at 4:19 AM Marek Szyprowski
> <[email protected]> wrote:
>> On 12.02.2024 00:09, Adam Ford 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 <[email protected]> # Kontron BL i.MX8MM with HDMI monitor
>>> Signed-off-by: Adam Ford <[email protected]>
>> Tested-by: Marek Szyprowski <[email protected]>
> Thank you very much for testing!
>
>>> ---
>>> V2: No changes
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index 8476650c477c..52939211fe93 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -1606,6 +1606,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) {
> Frieder & Marek S,
>
> Marek V is proposing we eliminate the check against the flags and do
> it unconditionally. If I send you both a different patch, would you
> be willing to try them on your platforms? I don't want to risk
> breaking a board.
I'm fine with testing it. I also have some additional spare boards to
replace the broken one, but so far none was bricked by my weird testing
activities.
> I used the check above from the NXP downstream kernel, so it felt
> safe, but I am not as familiar with the different DSI modes, so I am
> not sure what the impact would be if this read:
>
> if (dsi->lanes > 1) {
>
> Does anyone else have an opinion on this?
>>> + 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;
>>> }
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland