drivers/gpu/drm/bridge/fsl-ldb.c | 40 ++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)
LDB clock has to be a fixed multiple of the pixel clock.
As LDB and pixel clock are derived from different clock sources
(at least on imx8mp), this constraint cannot be satisfied for
any pixel clock, which leads to flickering and incomplete
lines on the attached display.
To overcome this, check this condition in mode_fixup() and
adapt the pixel clock accordingly.
Cc: <stable@vger.kernel.org>
Signed-off-by: Nikolaus Voss <nv@vosn.de>
---
drivers/gpu/drm/bridge/fsl-ldb.c | 40 ++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 0e4bac7dd04ff..e341341b8c600 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
return container_of(bridge, struct fsl_ldb, bridge);
}
+static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb *fsl_ldb)
+{
+ return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
+}
+
static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
{
- if (fsl_ldb_is_dual(fsl_ldb))
- return clock * 3500;
- else
- return clock * 7000;
+ return clock * fsl_ldb_link_freq_factor(fsl_ldb);
}
static int fsl_ldb_attach(struct drm_bridge *bridge,
@@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
bridge, flags);
}
+static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
+ const struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
+{
+ const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+ unsigned long requested_link_freq =
+ mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
+ unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq);
+
+ if (freq != requested_link_freq) {
+ /*
+ * this will lead to flicker and incomplete lines on
+ * the attached display, adjust the CRTC clock
+ * accordingly.
+ */
+ int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
+
+ if (adjusted_mode->clock != pclk) {
+ dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n",
+ adjusted_mode->clock, pclk);
+
+ adjusted_mode->clock = pclk;
+ adjusted_mode->crtc_clock = pclk;
+ }
+ }
+
+ return true;
+}
+
static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state)
{
@@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
static const struct drm_bridge_funcs funcs = {
.attach = fsl_ldb_attach,
+ .mode_fixup = fsl_ldb_mode_fixup,
.atomic_enable = fsl_ldb_atomic_enable,
.atomic_disable = fsl_ldb_atomic_disable,
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
--
2.43.0On 11/27/2024, Nikolaus Voss wrote:
> LDB clock has to be a fixed multiple of the pixel clock.
> As LDB and pixel clock are derived from different clock sources
> (at least on imx8mp), this constraint cannot be satisfied for
> any pixel clock, which leads to flickering and incomplete
> lines on the attached display.
>
> To overcome this, check this condition in mode_fixup() and
> adapt the pixel clock accordingly.
>
> Cc: <stable@vger.kernel.org>
It looks like stable is not effectively Cc'ed.
Need a Fixes tag?
>
> Signed-off-by: Nikolaus Voss <nv@vosn.de>
> ---
> drivers/gpu/drm/bridge/fsl-ldb.c | 40 ++++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 0e4bac7dd04ff..e341341b8c600 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
> return container_of(bridge, struct fsl_ldb, bridge);
> }
>
> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb *fsl_ldb)
> +{
> + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
> +}
> +
> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
> {
> - if (fsl_ldb_is_dual(fsl_ldb))
> - return clock * 3500;
> - else
> - return clock * 7000;
> + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
> }
>
> static int fsl_ldb_attach(struct drm_bridge *bridge,
> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
> bridge, flags);
> }
>
> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> +{
> + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
Why const?
If no const, then ...
> + unsigned long requested_link_freq =
> + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
... this could be
unsigned long requested_link_freq =
fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> + unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq);
> +
> + if (freq != requested_link_freq) {
> + /*
> + * this will lead to flicker and incomplete lines on
> + * the attached display, adjust the CRTC clock
> + * accordingly.
> + */
> + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
I doubt that pixel clock tree cannot find appropriate division ratios
for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
and i.MX93 platforms, because PLL clock rate should be 7x faster than
pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
folder between "ldb" clock and pixel clock can be met. That means the
PLL clock rate needs to be explicitly set first for this case.
Can you assign the PLL clock rate in DT to satisfy the "ldb" and pixel
clock rates like the below commit does, if you use a LVDS panel?
4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
frequency to 506.8 MHz")
> +
> + if (adjusted_mode->clock != pclk) {
> + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n",
> + adjusted_mode->clock, pclk);
> +
> + adjusted_mode->clock = pclk;
> + adjusted_mode->crtc_clock = pclk;
> + }
> + }
> +
> + return true;
> +}
> +
> static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_bridge_state)
> {
> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>
> static const struct drm_bridge_funcs funcs = {
> .attach = fsl_ldb_attach,
> + .mode_fixup = fsl_ldb_mode_fixup,
> .atomic_enable = fsl_ldb_atomic_enable,
> .atomic_disable = fsl_ldb_atomic_disable,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
--
Regards,
Liu Ying
On 12/2/24 7:32 AM, Liu Ying wrote: > On 11/27/2024, Nikolaus Voss wrote: >> LDB clock has to be a fixed multiple of the pixel clock. >> As LDB and pixel clock are derived from different clock sources >> (at least on imx8mp), this constraint cannot be satisfied for >> any pixel clock, which leads to flickering and incomplete >> lines on the attached display. >> >> To overcome this, check this condition in mode_fixup() and >> adapt the pixel clock accordingly. >> >> Cc: <stable@vger.kernel.org> > > It looks like stable is not effectively Cc'ed. > Need a Fixes tag? Isn't this fix effectively superseded by series [PATCH 0/5] clk: Fix simple video pipelines on i.MX8 ?
On 02.12.2024 13:56, Marek Vasut wrote: > On 12/2/24 7:32 AM, Liu Ying wrote: >> On 11/27/2024, Nikolaus Voss wrote: >>> LDB clock has to be a fixed multiple of the pixel clock. >>> As LDB and pixel clock are derived from different clock sources >>> (at least on imx8mp), this constraint cannot be satisfied for >>> any pixel clock, which leads to flickering and incomplete >>> lines on the attached display. >>> >>> To overcome this, check this condition in mode_fixup() and >>> adapt the pixel clock accordingly. >>> >>> Cc: <stable@vger.kernel.org> >> >> It looks like stable is not effectively Cc'ed. >> Need a Fixes tag? > Isn't this fix effectively superseded by series > > [PATCH 0/5] clk: Fix simple video pipelines on i.MX8 > > ? Maybe. I wasn't aware of the series. Looking at it, the change is rather complex and not suitable for the stable series. My intention was to get a simple fix which doesn't potentially break anything. It wouldn't even break the patch series you mentioned. -- Nikolaus Voss
On 12/2/24 6:03 PM, Nikolaus Voss wrote:
> On 02.12.2024 13:56, Marek Vasut wrote:
>> On 12/2/24 7:32 AM, Liu Ying wrote:
>>> On 11/27/2024, Nikolaus Voss wrote:
>>>> LDB clock has to be a fixed multiple of the pixel clock.
>>>> As LDB and pixel clock are derived from different clock sources
>>>> (at least on imx8mp), this constraint cannot be satisfied for
>>>> any pixel clock, which leads to flickering and incomplete
>>>> lines on the attached display.
>>>>
>>>> To overcome this, check this condition in mode_fixup() and
>>>> adapt the pixel clock accordingly.
>>>>
>>>> Cc: <stable@vger.kernel.org>
>>>
>>> It looks like stable is not effectively Cc'ed.
>>> Need a Fixes tag?
>> Isn't this fix effectively superseded by series
>>
>> [PATCH 0/5] clk: Fix simple video pipelines on i.MX8
>>
>> ?
>
> Maybe. I wasn't aware of the series. Looking at it, the change is
> rather complex and not suitable for the stable series.
>
> My intention was to get a simple fix which doesn't potentially
> break anything. It wouldn't even break the patch series you mentioned.
I know, the proper fix is indeed complex and not yet fully figured out.
The fix for stable for existing hardware is similar to this commit I think:
4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
frequency to 506.8 MHz")
Hi Liu,
On 02.12.2024 07:32, Liu Ying wrote:
> On 11/27/2024, Nikolaus Voss wrote:
>> LDB clock has to be a fixed multiple of the pixel clock.
>> As LDB and pixel clock are derived from different clock sources
>> (at least on imx8mp), this constraint cannot be satisfied for
>> any pixel clock, which leads to flickering and incomplete
>> lines on the attached display.
>>
>> To overcome this, check this condition in mode_fixup() and
>> adapt the pixel clock accordingly.
>>
>> Cc: <stable@vger.kernel.org>
>
> It looks like stable is not effectively Cc'ed.
> Need a Fixes tag?
I will include
Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale
i.MX8MP LDB bridge")
in v2.
>
>>
>> Signed-off-by: Nikolaus Voss <nv@vosn.de>
>> ---
>> drivers/gpu/drm/bridge/fsl-ldb.c | 40
>> ++++++++++++++++++++++++++++----
>> 1 file changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
>> b/drivers/gpu/drm/bridge/fsl-ldb.c
>> index 0e4bac7dd04ff..e341341b8c600 100644
>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>> @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct
>> drm_bridge *bridge)
>> return container_of(bridge, struct fsl_ldb, bridge);
>> }
>>
>> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb
>> *fsl_ldb)
>> +{
>> + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
>> +}
>> +
>> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb,
>> int clock)
>> {
>> - if (fsl_ldb_is_dual(fsl_ldb))
>> - return clock * 3500;
>> - else
>> - return clock * 7000;
>> + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
>> }
>>
>> static int fsl_ldb_attach(struct drm_bridge *bridge,
>> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge
>> *bridge,
>> bridge, flags);
>> }
>>
>> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
>> + const struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode)
>> +{
>> + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>
> Why const?
> If no const, then ...
>
>> + unsigned long requested_link_freq =
>> + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
>
> ... this could be
> unsigned long requested_link_freq =
>
> fsl_ldb_link_frequency(fsl_ldb,
> mode->clock);
I used fsl_ldb_link_freq_factor(fsl_ldb) to point out the symmetry to
recalculate
pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb) below:
>> + unsigned long freq = clk_round_rate(fsl_ldb->clk,
>> requested_link_freq);
>> +
>> + if (freq != requested_link_freq) {
>> + /*
>> + * this will lead to flicker and incomplete lines on
>> + * the attached display, adjust the CRTC clock
>> + * accordingly.
>> + */
>> + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
>
> I doubt that pixel clock tree cannot find appropriate division ratios
> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
> and i.MX93 platforms, because PLL clock rate should be 7x faster than
> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
> folder between "ldb" clock and pixel clock can be met. That means the
> PLL clock rate needs to be explicitly set first for this case.
>
> Can you assign the PLL clock rate in DT to satisfy the "ldb" and pixel
> clock rates like the below commit does, if you use a LVDS panel?
>
> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
> frequency to 506.8 MHz")
I probably could. The point of my patch is you don't have to know in
advance which LVDS panel is connected, and you don't have to calculate
the base PLL clock by hand and store it in the device tree.
In my test system, I have three different LVDS panels with EDID EEPROM,
none of which worked with the stock driver, but all work with this
patch.
With slightly adapted pixel clocks though.
The driver already warns if LDB and PCLK don't match, because this is a
hardware constraint and violating this will lead to no image or image
distortion. With the patch the driver tries to fix that.
I don't know if it works for all or at least most panels, but for my
panels
it does. And if the clocks match, by chance or by refconfiguring the PLL
base
clock, the patch does nothing.
>
>> +
>> + if (adjusted_mode->clock != pclk) {
>> + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d
>> kHz -> %d kHz)!\n",
>> + adjusted_mode->clock, pclk);
>> +
>> + adjusted_mode->clock = pclk;
>> + adjusted_mode->crtc_clock = pclk;
>> + }
>> + }
>> +
>> + return true;
>> +}
>> +
>> static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>> struct drm_bridge_state *old_bridge_state)
>> {
>> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>>
>> static const struct drm_bridge_funcs funcs = {
>> .attach = fsl_ldb_attach,
>> + .mode_fixup = fsl_ldb_mode_fixup,
>> .atomic_enable = fsl_ldb_atomic_enable,
>> .atomic_disable = fsl_ldb_atomic_disable,
>> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> --
> Regards,
> Liu Ying
--
Nikolaus Voss
Am Montag, dem 02.12.2024 um 11:13 +0100 schrieb Nikolaus Voss:
> Hi Liu,
>
> On 02.12.2024 07:32, Liu Ying wrote:
> > On 11/27/2024, Nikolaus Voss wrote:
> > > LDB clock has to be a fixed multiple of the pixel clock.
> > > As LDB and pixel clock are derived from different clock sources
> > > (at least on imx8mp), this constraint cannot be satisfied for
> > > any pixel clock, which leads to flickering and incomplete
> > > lines on the attached display.
> > >
> > > To overcome this, check this condition in mode_fixup() and
> > > adapt the pixel clock accordingly.
> > >
> > > Cc: <stable@vger.kernel.org>
> >
> > It looks like stable is not effectively Cc'ed.
> > Need a Fixes tag?
>
> I will include
> Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale
> i.MX8MP LDB bridge")
> in v2.
>
> >
> > >
> > > Signed-off-by: Nikolaus Voss <nv@vosn.de>
> > > ---
> > > drivers/gpu/drm/bridge/fsl-ldb.c | 40
> > > ++++++++++++++++++++++++++++----
> > > 1 file changed, 36 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
> > > b/drivers/gpu/drm/bridge/fsl-ldb.c
> > > index 0e4bac7dd04ff..e341341b8c600 100644
> > > --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> > > +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> > > @@ -104,12 +104,14 @@ static inline struct fsl_ldb
> > > *to_fsl_ldb(struct
> > > drm_bridge *bridge)
> > > return container_of(bridge, struct fsl_ldb, bridge);
> > > }
> > >
> > > +static unsigned int fsl_ldb_link_freq_factor(const struct
> > > fsl_ldb
> > > *fsl_ldb)
> > > +{
> > > + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
> > > +}
> > > +
> > > static unsigned long fsl_ldb_link_frequency(struct fsl_ldb
> > > *fsl_ldb,
> > > int clock)
> > > {
> > > - if (fsl_ldb_is_dual(fsl_ldb))
> > > - return clock * 3500;
> > > - else
> > > - return clock * 7000;
> > > + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
> > > }
> > >
> > > static int fsl_ldb_attach(struct drm_bridge *bridge,
> > > @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge
> > > *bridge,
> > > bridge, flags);
> > > }
> > >
> > > +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
> > > + const struct drm_display_mode
> > > *mode,
> > > + struct drm_display_mode
> > > *adjusted_mode)
> > > +{
> > > + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> >
> > Why const?
> > If no const, then ...
> >
> > > + unsigned long requested_link_freq =
> > > + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
> >
> > ... this could be
> > unsigned long requested_link_freq =
> >
> > fsl_ldb_link_frequency(fsl_ldb,
> > mode->clock);
>
> I used fsl_ldb_link_freq_factor(fsl_ldb) to point out the symmetry to
> recalculate
> pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb) below:
>
> > > + unsigned long freq = clk_round_rate(fsl_ldb->clk,
> > > requested_link_freq);
> > > +
> > > + if (freq != requested_link_freq) {
> > > + /*
> > > + * this will lead to flicker and incomplete lines
> > > on
> > > + * the attached display, adjust the CRTC clock
> > > + * accordingly.
> > > + */
> > > + int pclk = freq /
> > > fsl_ldb_link_freq_factor(fsl_ldb);
> >
> > I doubt that pixel clock tree cannot find appropriate division
> > ratios
> > for some pixel clock rates, especially for dual-link LVDS on
> > i.MX8MP
> > and i.MX93 platforms, because PLL clock rate should be 7x faster
> > than
> > pixel clock rate and 2x faster than "ldb" clock rate so that the
> > 3.5
> > folder between "ldb" clock and pixel clock can be met. That means
> > the
> > PLL clock rate needs to be explicitly set first for this case.
> >
> > Can you assign the PLL clock rate in DT to satisfy the "ldb" and
> > pixel
> > clock rates like the below commit does, if you use a LVDS panel?
> >
> > 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
> > frequency to 506.8 MHz")
>
> I probably could. The point of my patch is you don't have to know in
> advance which LVDS panel is connected, and you don't have to
> calculate
> the base PLL clock by hand and store it in the device tree.
>
> In my test system, I have three different LVDS panels with EDID
> EEPROM,
> none of which worked with the stock driver, but all work with this
> patch.
> With slightly adapted pixel clocks though.
>
> The driver already warns if LDB and PCLK don't match, because this is
> a
> hardware constraint and violating this will lead to no image or image
> distortion. With the patch the driver tries to fix that.
>
> I don't know if it works for all or at least most panels, but for my
> panels
> it does. And if the clocks match, by chance or by refconfiguring the
> PLL
> base
> clock, the patch does nothing.
just as a data point, I'm very happy with this patch as it make my 51,2
MHz lvds panel just work without doing anything else. I'll happily
carry it along for v6.12 if it's not going into stable :)
martin
On 12/02/2024, Nikolaus Voss wrote:
> [You don't often get email from nv@vosn.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Liu,
Hi,
>
> On 02.12.2024 07:32, Liu Ying wrote:
>> On 11/27/2024, Nikolaus Voss wrote:
>>> LDB clock has to be a fixed multiple of the pixel clock.
>>> As LDB and pixel clock are derived from different clock sources
>>> (at least on imx8mp), this constraint cannot be satisfied for
>>> any pixel clock, which leads to flickering and incomplete
>>> lines on the attached display.
>>>
>>> To overcome this, check this condition in mode_fixup() and
>>> adapt the pixel clock accordingly.
>>>
>>> Cc: <stable@vger.kernel.org>
>>
>> It looks like stable is not effectively Cc'ed.
>> Need a Fixes tag?
>
> I will include
> Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale
> i.MX8MP LDB bridge")
> in v2.
>
>>
>>>
>>> Signed-off-by: Nikolaus Voss <nv@vosn.de>
>>> ---
>>> drivers/gpu/drm/bridge/fsl-ldb.c | 40
>>> ++++++++++++++++++++++++++++----
>>> 1 file changed, 36 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
>>> b/drivers/gpu/drm/bridge/fsl-ldb.c
>>> index 0e4bac7dd04ff..e341341b8c600 100644
>>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>>> @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct
>>> drm_bridge *bridge)
>>> return container_of(bridge, struct fsl_ldb, bridge);
>>> }
>>>
>>> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb
>>> *fsl_ldb)
>>> +{
>>> + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
>>> +}
>>> +
>>> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb,
>>> int clock)
>>> {
>>> - if (fsl_ldb_is_dual(fsl_ldb))
>>> - return clock * 3500;
>>> - else
>>> - return clock * 7000;
>>> + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
>>> }
>>>
>>> static int fsl_ldb_attach(struct drm_bridge *bridge,
>>> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge
>>> *bridge,
>>> bridge, flags);
>>> }
>>>
>>> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
>>> + const struct drm_display_mode *mode,
>>> + struct drm_display_mode *adjusted_mode)
>>> +{
>>> + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>>
>> Why const?
>> If no const, then ...
>>
>>> + unsigned long requested_link_freq =
>>> + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
>>
>> ... this could be
>> unsigned long requested_link_freq =
>>
>> fsl_ldb_link_frequency(fsl_ldb,
>> mode->clock);
>
> I used fsl_ldb_link_freq_factor(fsl_ldb) to point out the symmetry to
> recalculate
> pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb) below:
I would prefer to avoid this open coding of using
fsl_ldb_link_freq_factor() than achieve some sort of symmetry.
>
>>> + unsigned long freq = clk_round_rate(fsl_ldb->clk,
>>> requested_link_freq);
>>> +
>>> + if (freq != requested_link_freq) {
>>> + /*
>>> + * this will lead to flicker and incomplete lines on
>>> + * the attached display, adjust the CRTC clock
>>> + * accordingly.
>>> + */
>>> + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
>>
>> I doubt that pixel clock tree cannot find appropriate division ratios
>> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
>> and i.MX93 platforms, because PLL clock rate should be 7x faster than
>> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
>> folder between "ldb" clock and pixel clock can be met. That means the
>> PLL clock rate needs to be explicitly set first for this case.
>>
>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and pixel
>> clock rates like the below commit does, if you use a LVDS panel?
>>
>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>> frequency to 506.8 MHz")
>
> I probably could. The point of my patch is you don't have to know in
> advance which LVDS panel is connected, and you don't have to calculate
> the base PLL clock by hand and store it in the device tree.
>
> In my test system, I have three different LVDS panels with EDID EEPROM,
> none of which worked with the stock driver, but all work with this
> patch.
> With slightly adapted pixel clocks though.
If each of the three LVDS panels has only one display mode, you may
assign the PLL clock rates in DT overlays for the panels.
>
> The driver already warns if LDB and PCLK don't match, because this is a
> hardware constraint and violating this will lead to no image or image
> distortion. With the patch the driver tries to fix that.
>
> I don't know if it works for all or at least most panels, but for my
This patch is supposed to work for all panels and LVDS display bridges,
but I don't think it does due to the problematic pixel clock rates I
mentioned before.
> panels
> it does. And if the clocks match, by chance or by refconfiguring the PLL
> base
> clock, the patch does nothing.
>
>>
>>> +
>>> + if (adjusted_mode->clock != pclk) {
>>> + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d
>>> kHz -> %d kHz)!\n",
>>> + adjusted_mode->clock, pclk);
>>> +
>>> + adjusted_mode->clock = pclk;
>>> + adjusted_mode->crtc_clock = pclk;
>>> + }
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>>> struct drm_bridge_state *old_bridge_state)
>>> {
>>> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>>>
>>> static const struct drm_bridge_funcs funcs = {
>>> .attach = fsl_ldb_attach,
>>> + .mode_fixup = fsl_ldb_mode_fixup,
>>> .atomic_enable = fsl_ldb_atomic_enable,
>>> .atomic_disable = fsl_ldb_atomic_disable,
>>> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>> --
>> Regards,
>> Liu Ying
>
> --
> Nikolaus Voss
--
Regards,
Liu Ying
On 03.12.2024 03:22, Liu Ying wrote:
> On 12/02/2024, Nikolaus Voss wrote:
>> [You don't often get email from nv@vosn.de. Learn why this is
>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Hi Liu,
>
> Hi,
>
>>
>> On 02.12.2024 07:32, Liu Ying wrote:
>>> On 11/27/2024, Nikolaus Voss wrote:
>>>> LDB clock has to be a fixed multiple of the pixel clock.
>>>> As LDB and pixel clock are derived from different clock sources
>>>> (at least on imx8mp), this constraint cannot be satisfied for
>>>> any pixel clock, which leads to flickering and incomplete
>>>> lines on the attached display.
>>>>
>>>> To overcome this, check this condition in mode_fixup() and
>>>> adapt the pixel clock accordingly.
>>>>
>>>> Cc: <stable@vger.kernel.org>
>>>
>>> It looks like stable is not effectively Cc'ed.
>>> Need a Fixes tag?
>>
>> I will include
>> Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale
>> i.MX8MP LDB bridge")
>> in v2.
>>
>>>
>>>>
>>>> Signed-off-by: Nikolaus Voss <nv@vosn.de>
>>>> ---
>>>> drivers/gpu/drm/bridge/fsl-ldb.c | 40
>>>> ++++++++++++++++++++++++++++----
>>>> 1 file changed, 36 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
>>>> b/drivers/gpu/drm/bridge/fsl-ldb.c
>>>> index 0e4bac7dd04ff..e341341b8c600 100644
>>>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>>>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>>>> @@ -104,12 +104,14 @@ static inline struct fsl_ldb
>>>> *to_fsl_ldb(struct
>>>> drm_bridge *bridge)
>>>> return container_of(bridge, struct fsl_ldb, bridge);
>>>> }
>>>>
>>>> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb
>>>> *fsl_ldb)
>>>> +{
>>>> + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
>>>> +}
>>>> +
>>>> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb
>>>> *fsl_ldb,
>>>> int clock)
>>>> {
>>>> - if (fsl_ldb_is_dual(fsl_ldb))
>>>> - return clock * 3500;
>>>> - else
>>>> - return clock * 7000;
>>>> + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
>>>> }
>>>>
>>>> static int fsl_ldb_attach(struct drm_bridge *bridge,
>>>> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge
>>>> *bridge,
>>>> bridge, flags);
>>>> }
>>>>
>>>> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
>>>> + const struct drm_display_mode *mode,
>>>> + struct drm_display_mode *adjusted_mode)
>>>> +{
>>>> + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>>>
>>> Why const?
>>> If no const, then ...
>>>
>>>> + unsigned long requested_link_freq =
>>>> + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
>>>
>>> ... this could be
>>> unsigned long requested_link_freq =
>>>
>>> fsl_ldb_link_frequency(fsl_ldb,
>>> mode->clock);
>>
>> I used fsl_ldb_link_freq_factor(fsl_ldb) to point out the symmetry to
>> recalculate
>> pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb) below:
>
> I would prefer to avoid this open coding of using
> fsl_ldb_link_freq_factor() than achieve some sort of symmetry.
>
>>
>>>> + unsigned long freq = clk_round_rate(fsl_ldb->clk,
>>>> requested_link_freq);
>>>> +
>>>> + if (freq != requested_link_freq) {
>>>> + /*
>>>> + * this will lead to flicker and incomplete lines on
>>>> + * the attached display, adjust the CRTC clock
>>>> + * accordingly.
>>>> + */
>>>> + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
>>>
>>> I doubt that pixel clock tree cannot find appropriate division ratios
>>> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
>>> and i.MX93 platforms, because PLL clock rate should be 7x faster than
>>> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
>>> folder between "ldb" clock and pixel clock can be met. That means the
>>> PLL clock rate needs to be explicitly set first for this case.
>>>
>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and
>>> pixel
>>> clock rates like the below commit does, if you use a LVDS panel?
>>>
>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>>> frequency to 506.8 MHz")
>>
>> I probably could. The point of my patch is you don't have to know in
>> advance which LVDS panel is connected, and you don't have to calculate
>> the base PLL clock by hand and store it in the device tree.
>>
>> In my test system, I have three different LVDS panels with EDID
>> EEPROM,
>> none of which worked with the stock driver, but all work with this
>> patch.
>> With slightly adapted pixel clocks though.
>
> If each of the three LVDS panels has only one display mode, you may
> assign the PLL clock rates in DT overlays for the panels.
Yes, but I would have to calculate the PLL clock by hand with knowledge
of the internal signal flow (at least the clock ratio ;-)).
>
>>
>> The driver already warns if LDB and PCLK don't match, because this is
>> a
>> hardware constraint and violating this will lead to no image or image
>> distortion. With the patch the driver tries to fix that.
>>
>> I don't know if it works for all or at least most panels, but for my
>
> This patch is supposed to work for all panels and LVDS display bridges,
> but I don't think it does due to the problematic pixel clock rates I
> mentioned before.
To my understanding, the current driver works for nearly no panel,
unless
you explicitly code the correct PLL freq into the device tree. The basic
problem is, that the driver does not _enforce_ the correct clock ratio
although the hardware strictly _requires_ it.
There are two options in enforcing the correct ratio: forcing the ldb
clock
to the right rate, down to the clock source; this is Miquel's approach.
Or getting the nearest possible ldb clock with the current PLL freq and
modify the pixel clock accordingly, that's my approach.
Of course, never modifying the required pixel clock and still setting
the
correct clock ratio is the golden solution. But in most cases, this is
not
necessary as panels usually accept deviations from the ideal pixel clock
without any problems. What doesn't work is pixel and ldb clock not
having
the correct ratio. And this is what the patch is about.
>
>> panels
>> it does. And if the clocks match, by chance or by refconfiguring the
>> PLL
>> base
>> clock, the patch does nothing.
>>
>>>
>>>> +
>>>> + if (adjusted_mode->clock != pclk) {
>>>> + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to
>>>> match LDB clk (%d
>>>> kHz -> %d kHz)!\n",
>>>> + adjusted_mode->clock, pclk);
>>>> +
>>>> + adjusted_mode->clock = pclk;
>>>> + adjusted_mode->crtc_clock = pclk;
>>>> + }
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>>>> struct drm_bridge_state
>>>> *old_bridge_state)
>>>> {
>>>> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>>>>
>>>> static const struct drm_bridge_funcs funcs = {
>>>> .attach = fsl_ldb_attach,
>>>> + .mode_fixup = fsl_ldb_mode_fixup,
>>>> .atomic_enable = fsl_ldb_atomic_enable,
>>>> .atomic_disable = fsl_ldb_atomic_disable,
>>>> .atomic_duplicate_state =
>>>> drm_atomic_helper_bridge_duplicate_state,
--
Nikolaus Voss
On 12/03/2024, Nikolaus Voss wrote:
> On 03.12.2024 03:22, Liu Ying wrote:
>> On 12/02/2024, Nikolaus Voss wrote:
>>> [You don't often get email from nv@vosn.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> Hi Liu,
>>
>> Hi,
>>
>>>
>>> On 02.12.2024 07:32, Liu Ying wrote:
>>>> On 11/27/2024, Nikolaus Voss wrote:
>>>>> LDB clock has to be a fixed multiple of the pixel clock.
>>>>> As LDB and pixel clock are derived from different clock sources
>>>>> (at least on imx8mp), this constraint cannot be satisfied for
>>>>> any pixel clock, which leads to flickering and incomplete
>>>>> lines on the attached display.
>>>>>
>>>>> To overcome this, check this condition in mode_fixup() and
>>>>> adapt the pixel clock accordingly.
>>>>>
>>>>> Cc: <stable@vger.kernel.org>
>>>>
>>>> It looks like stable is not effectively Cc'ed.
>>>> Need a Fixes tag?
>>>
>>> I will include
>>> Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale
>>> i.MX8MP LDB bridge")
>>> in v2.
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Nikolaus Voss <nv@vosn.de>
>>>>> ---
>>>>> drivers/gpu/drm/bridge/fsl-ldb.c | 40
>>>>> ++++++++++++++++++++++++++++----
>>>>> 1 file changed, 36 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
>>>>> b/drivers/gpu/drm/bridge/fsl-ldb.c
>>>>> index 0e4bac7dd04ff..e341341b8c600 100644
>>>>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>>>>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>>>>> @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct
>>>>> drm_bridge *bridge)
>>>>> return container_of(bridge, struct fsl_ldb, bridge);
>>>>> }
>>>>>
>>>>> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb
>>>>> *fsl_ldb)
>>>>> +{
>>>>> + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
>>>>> +}
>>>>> +
>>>>> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb,
>>>>> int clock)
>>>>> {
>>>>> - if (fsl_ldb_is_dual(fsl_ldb))
>>>>> - return clock * 3500;
>>>>> - else
>>>>> - return clock * 7000;
>>>>> + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
>>>>> }
>>>>>
>>>>> static int fsl_ldb_attach(struct drm_bridge *bridge,
>>>>> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge
>>>>> *bridge,
>>>>> bridge, flags);
>>>>> }
>>>>>
>>>>> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
>>>>> + const struct drm_display_mode *mode,
>>>>> + struct drm_display_mode *adjusted_mode)
>>>>> +{
>>>>> + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>>>>
>>>> Why const?
>>>> If no const, then ...
>>>>
>>>>> + unsigned long requested_link_freq =
>>>>> + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
>>>>
>>>> ... this could be
>>>> unsigned long requested_link_freq =
>>>>
>>>> fsl_ldb_link_frequency(fsl_ldb,
>>>> mode->clock);
>>>
>>> I used fsl_ldb_link_freq_factor(fsl_ldb) to point out the symmetry to
>>> recalculate
>>> pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb) below:
>>
>> I would prefer to avoid this open coding of using
>> fsl_ldb_link_freq_factor() than achieve some sort of symmetry.
>>
>>>
>>>>> + unsigned long freq = clk_round_rate(fsl_ldb->clk,
>>>>> requested_link_freq);
>>>>> +
>>>>> + if (freq != requested_link_freq) {
>>>>> + /*
>>>>> + * this will lead to flicker and incomplete lines on
>>>>> + * the attached display, adjust the CRTC clock
>>>>> + * accordingly.
>>>>> + */
>>>>> + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
>>>>
>>>> I doubt that pixel clock tree cannot find appropriate division ratios
>>>> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
>>>> and i.MX93 platforms, because PLL clock rate should be 7x faster than
>>>> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
>>>> folder between "ldb" clock and pixel clock can be met. That means the
>>>> PLL clock rate needs to be explicitly set first for this case.
>>>>
>>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and pixel
>>>> clock rates like the below commit does, if you use a LVDS panel?
>>>>
>>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>>>> frequency to 506.8 MHz")
>>>
>>> I probably could. The point of my patch is you don't have to know in
>>> advance which LVDS panel is connected, and you don't have to calculate
>>> the base PLL clock by hand and store it in the device tree.
>>>
>>> In my test system, I have three different LVDS panels with EDID EEPROM,
>>> none of which worked with the stock driver, but all work with this
>>> patch.
>>> With slightly adapted pixel clocks though.
>>
>> If each of the three LVDS panels has only one display mode, you may
>> assign the PLL clock rates in DT overlays for the panels.
>
> Yes, but I would have to calculate the PLL clock by hand with knowledge
> of the internal signal flow (at least the clock ratio ;-)).
To support a shared PLL between LVDS display pipeline and MIPI DSI display
pipeline with i.MX8MP + separate PLLs, I'm afraid you have to assign the
PLL clock rate(s) in DT with knowledge of the clock ratios. See my patch
series for details.
https://lore.kernel.org/all/20241114065759.3341908-1-victor.liu@nxp.com/
>
>>
>>>
>>> The driver already warns if LDB and PCLK don't match, because this is a
>>> hardware constraint and violating this will lead to no image or image
>>> distortion. With the patch the driver tries to fix that.
>>>
>>> I don't know if it works for all or at least most panels, but for my
>>
>> This patch is supposed to work for all panels and LVDS display bridges,
>> but I don't think it does due to the problematic pixel clock rates I
>> mentioned before.
>
> To my understanding, the current driver works for nearly no panel, unless
> you explicitly code the correct PLL freq into the device tree. The basic
> problem is, that the driver does not _enforce_ the correct clock ratio
> although the hardware strictly _requires_ it.
My above reply applies here, too.
>
> There are two options in enforcing the correct ratio: forcing the ldb clock
> to the right rate, down to the clock source; this is Miquel's approach.
> Or getting the nearest possible ldb clock with the current PLL freq and
> modify the pixel clock accordingly, that's my approach.
With the PLL clock rate(s) assigned in DT, the pixel clock and LDB clock
are kind of deterministic.
>
> Of course, never modifying the required pixel clock and still setting the
> correct clock ratio is the golden solution. But in most cases, this is not
> necessary as panels usually accept deviations from the ideal pixel clock
> without any problems. What doesn't work is pixel and ldb clock not having
> the correct ratio. And this is what the patch is about.
As I said before, this patch is supposed to work for all panels and LVDS
display bridges, but I don't think it does. You may try to change your
single-link LVDS panels to virtual dual-link LVDS panels in DT by adding
the second LVDS link. I don't think the pixel clock rate and/or LDB clock
rate after fixup would work for the virtual panels.
Looking into your patch more carefully, it blindly fixes the pixel clock
rate up, which means the PLL clock rate might be changed when LCDIF driver
sets the fixed-up pixel clock rate because the pixel clock
(IMX8MP_CLK_MEDIA_DISP2_PIX and IMX8MP_CLK_MEDIA_DISP2_PIX_ROOT) has
the CLK_SET_RATE_PARENT flag set. That means the previous rounded LDB
clock rate gotten from clk_round_rate() function call against fsl_ldb->clk
might works no more.
Note that the mode_fixup/atomic_check callbacks should validate every
atomic commit correctly. Once the commit passes the check successfully,
the commit should just work. But, it looks like your patch might fail to
filter out invalid atomic commits due to the clock constraints.
>
>>
>>> panels
>>> it does. And if the clocks match, by chance or by refconfiguring the PLL
>>> base
>>> clock, the patch does nothing.
>>>
>>>>
>>>>> +
>>>>> + if (adjusted_mode->clock != pclk) {
>>>>> + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d
>>>>> kHz -> %d kHz)!\n",
>>>>> + adjusted_mode->clock, pclk);
>>>>> +
>>>>> + adjusted_mode->clock = pclk;
>>>>> + adjusted_mode->crtc_clock = pclk;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>>>>> struct drm_bridge_state *old_bridge_state)
>>>>> {
>>>>> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>>>>>
>>>>> static const struct drm_bridge_funcs funcs = {
>>>>> .attach = fsl_ldb_attach,
>>>>> + .mode_fixup = fsl_ldb_mode_fixup,
>>>>> .atomic_enable = fsl_ldb_atomic_enable,
>>>>> .atomic_disable = fsl_ldb_atomic_disable,
>>>>> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>
--
Regards,
Liu Ying
On 12/3/24 3:22 AM, Liu Ying wrote:
[...]
>>> I doubt that pixel clock tree cannot find appropriate division ratios
>>> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
>>> and i.MX93 platforms, because PLL clock rate should be 7x faster than
>>> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
>>> folder between "ldb" clock and pixel clock can be met. That means the
>>> PLL clock rate needs to be explicitly set first for this case.
>>>
>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and pixel
>>> clock rates like the below commit does, if you use a LVDS panel?
>>>
>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>>> frequency to 506.8 MHz")
>>
>> I probably could. The point of my patch is you don't have to know in
>> advance which LVDS panel is connected, and you don't have to calculate
>> the base PLL clock by hand and store it in the device tree.
>>
>> In my test system, I have three different LVDS panels with EDID EEPROM,
>> none of which worked with the stock driver, but all work with this
>> patch.
>> With slightly adapted pixel clocks though.
>
> If each of the three LVDS panels has only one display mode, you may
> assign the PLL clock rates in DT overlays for the panels.
I temporarily agree.
I also currently use DTOs for various panels including their PLL
setting, but in the end, I think/hope the work of Miquel and co. is
going to make that PLL setting part unnecessary.
On 03.12.2024 04:12, Marek Vasut wrote:
> On 12/3/24 3:22 AM, Liu Ying wrote:
>
> [...]
>
>>>> I doubt that pixel clock tree cannot find appropriate division
>>>> ratios
>>>> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
>>>> and i.MX93 platforms, because PLL clock rate should be 7x faster
>>>> than
>>>> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
>>>> folder between "ldb" clock and pixel clock can be met. That means
>>>> the
>>>> PLL clock rate needs to be explicitly set first for this case.
>>>>
>>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and
>>>> pixel
>>>> clock rates like the below commit does, if you use a LVDS panel?
>>>>
>>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>>>> frequency to 506.8 MHz")
>>>
>>> I probably could. The point of my patch is you don't have to know in
>>> advance which LVDS panel is connected, and you don't have to
>>> calculate
>>> the base PLL clock by hand and store it in the device tree.
>>>
>>> In my test system, I have three different LVDS panels with EDID
>>> EEPROM,
>>> none of which worked with the stock driver, but all work with this
>>> patch.
>>> With slightly adapted pixel clocks though.
>>
>> If each of the three LVDS panels has only one display mode, you may
>> assign the PLL clock rates in DT overlays for the panels.
> I temporarily agree.
>
> I also currently use DTOs for various panels including their PLL
> setting, but in the end, I think/hope the work of Miquel and co. is
> going to make that PLL setting part unnecessary.
That is exactly what my patch is about. I want to use one DT for all
panels and store the panel's timing in EDID EEPROM.
--
Nikolaus Voss
On 12/3/24 8:20 AM, Nikolaus Voss wrote:
> On 03.12.2024 04:12, Marek Vasut wrote:
>> On 12/3/24 3:22 AM, Liu Ying wrote:
>>
>> [...]
>>
>>>>> I doubt that pixel clock tree cannot find appropriate division ratios
>>>>> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
>>>>> and i.MX93 platforms, because PLL clock rate should be 7x faster than
>>>>> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
>>>>> folder between "ldb" clock and pixel clock can be met. That means the
>>>>> PLL clock rate needs to be explicitly set first for this case.
>>>>>
>>>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and pixel
>>>>> clock rates like the below commit does, if you use a LVDS panel?
>>>>>
>>>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>>>>> frequency to 506.8 MHz")
>>>>
>>>> I probably could. The point of my patch is you don't have to know in
>>>> advance which LVDS panel is connected, and you don't have to calculate
>>>> the base PLL clock by hand and store it in the device tree.
>>>>
>>>> In my test system, I have three different LVDS panels with EDID EEPROM,
>>>> none of which worked with the stock driver, but all work with this
>>>> patch.
>>>> With slightly adapted pixel clocks though.
>>>
>>> If each of the three LVDS panels has only one display mode, you may
>>> assign the PLL clock rates in DT overlays for the panels.
>> I temporarily agree.
>>
>> I also currently use DTOs for various panels including their PLL
>> setting, but in the end, I think/hope the work of Miquel and co. is
>> going to make that PLL setting part unnecessary.
>
> That is exactly what my patch is about. I want to use one DT for all
> panels
Right
> and store the panel's timing in EDID EEPROM.
Oh, that is a new one. Does the EDID EEPROM store the entirety of
'struct display_timing {}' somehow , or is that a custom format ?
Hi Marek,
On 04.12.2024 00:40, Marek Vasut wrote:
> On 12/3/24 8:20 AM, Nikolaus Voss wrote:
>> On 03.12.2024 04:12, Marek Vasut wrote:
>>> On 12/3/24 3:22 AM, Liu Ying wrote:
>>>
>>> [...]
>>>
>>>>>> I doubt that pixel clock tree cannot find appropriate division
>>>>>> ratios
>>>>>> for some pixel clock rates, especially for dual-link LVDS on
>>>>>> i.MX8MP
>>>>>> and i.MX93 platforms, because PLL clock rate should be 7x faster
>>>>>> than
>>>>>> pixel clock rate and 2x faster than "ldb" clock rate so that the
>>>>>> 3.5
>>>>>> folder between "ldb" clock and pixel clock can be met. That means
>>>>>> the
>>>>>> PLL clock rate needs to be explicitly set first for this case.
>>>>>>
>>>>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and
>>>>>> pixel
>>>>>> clock rates like the below commit does, if you use a LVDS panel?
>>>>>>
>>>>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>>>>>> frequency to 506.8 MHz")
>>>>>
>>>>> I probably could. The point of my patch is you don't have to know
>>>>> in
>>>>> advance which LVDS panel is connected, and you don't have to
>>>>> calculate
>>>>> the base PLL clock by hand and store it in the device tree.
>>>>>
>>>>> In my test system, I have three different LVDS panels with EDID
>>>>> EEPROM,
>>>>> none of which worked with the stock driver, but all work with this
>>>>> patch.
>>>>> With slightly adapted pixel clocks though.
>>>>
>>>> If each of the three LVDS panels has only one display mode, you may
>>>> assign the PLL clock rates in DT overlays for the panels.
>>> I temporarily agree.
>>>
>>> I also currently use DTOs for various panels including their PLL
>>> setting, but in the end, I think/hope the work of Miquel and co. is
>>> going to make that PLL setting part unnecessary.
>>
>> That is exactly what my patch is about. I want to use one DT for all
>> panels
>
> Right
>
>> and store the panel's timing in EDID EEPROM.
> Oh, that is a new one. Does the EDID EEPROM store the entirety of
> 'struct display_timing {}' somehow , or is that a custom format ?
Well, sort of ;-). VESA has taken care of this 30 years ago
(https://en.wikipedia.org/wiki/Extended_Display_Identification_Data).
DRM handles this with drm_get_edid() and siblings, e.g. :
@@ -86,16 +92,36 @@ static int panel_lvds_get_modes(struct drm_panel
*panel,
{
struct panel_lvds *lvds = to_panel_lvds(panel);
struct drm_display_mode *mode;
+ int num = 0;
+
+ /* probe EDID if a DDC bus is available */
+ if (lvds->ddc) {
+ pm_runtime_get_sync(lvds->dev);
+
+ if (!lvds->edid)
+ lvds->edid = drm_get_edid(connector, lvds->ddc);
+
+ if (lvds->edid)
+ num += drm_add_edid_modes(connector,
lvds->edid);
+
+ pm_runtime_mark_last_busy(lvds->dev);
+ pm_runtime_put_autosuspend(lvds->dev);
+ }
panel-simple.c does that in mainline, I added it to panel-lvds.c.
The kernel subdir tools/edid has some code to generate the EEPROM data
from timings and flags.
We keep the DDC EEPROM on a small adapter glued to to back of the panel
so we can replace the usually short-lived panel with a successor.
--
Nikolaus Voss
On 12/4/24 11:55 AM, Nikolaus Voss wrote:
> Hi Marek,
Hi,
>>>>>>> I doubt that pixel clock tree cannot find appropriate division
>>>>>>> ratios
>>>>>>> for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
>>>>>>> and i.MX93 platforms, because PLL clock rate should be 7x faster
>>>>>>> than
>>>>>>> pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
>>>>>>> folder between "ldb" clock and pixel clock can be met. That means
>>>>>>> the
>>>>>>> PLL clock rate needs to be explicitly set first for this case.
>>>>>>>
>>>>>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and
>>>>>>> pixel
>>>>>>> clock rates like the below commit does, if you use a LVDS panel?
>>>>>>>
>>>>>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>>>>>>> frequency to 506.8 MHz")
>>>>>>
>>>>>> I probably could. The point of my patch is you don't have to know in
>>>>>> advance which LVDS panel is connected, and you don't have to
>>>>>> calculate
>>>>>> the base PLL clock by hand and store it in the device tree.
>>>>>>
>>>>>> In my test system, I have three different LVDS panels with EDID
>>>>>> EEPROM,
>>>>>> none of which worked with the stock driver, but all work with this
>>>>>> patch.
>>>>>> With slightly adapted pixel clocks though.
>>>>>
>>>>> If each of the three LVDS panels has only one display mode, you may
>>>>> assign the PLL clock rates in DT overlays for the panels.
>>>> I temporarily agree.
>>>>
>>>> I also currently use DTOs for various panels including their PLL
>>>> setting, but in the end, I think/hope the work of Miquel and co. is
>>>> going to make that PLL setting part unnecessary.
>>>
>>> That is exactly what my patch is about. I want to use one DT for all
>>> panels
>>
>> Right
>>
>>> and store the panel's timing in EDID EEPROM.
>> Oh, that is a new one. Does the EDID EEPROM store the entirety of
>> 'struct display_timing {}' somehow , or is that a custom format ?
>
> Well, sort of ;-). VESA has taken care of this 30 years ago
> (https://en.wikipedia.org/wiki/Extended_Display_Identification_Data).
>
> DRM handles this with drm_get_edid() and siblings, e.g. :
EDID can not encode all the information in struct display_timing {} , or
can it ?
I think what you would be missing are bus_flags , bus_format and
possibly the single/dual link and channel (odd/even) mapping, won't you ?
> @@ -86,16 +92,36 @@ static int panel_lvds_get_modes(struct drm_panel
> *panel,
> {
> struct panel_lvds *lvds = to_panel_lvds(panel);
> struct drm_display_mode *mode;
> + int num = 0;
> +
> + /* probe EDID if a DDC bus is available */
> + if (lvds->ddc) {
> + pm_runtime_get_sync(lvds->dev);
> +
> + if (!lvds->edid)
> + lvds->edid = drm_get_edid(connector, lvds->ddc);
> +
> + if (lvds->edid)
> + num += drm_add_edid_modes(connector, lvds->edid);
> +
> + pm_runtime_mark_last_busy(lvds->dev);
> + pm_runtime_put_autosuspend(lvds->dev);
> + }
>
> panel-simple.c does that in mainline, I added it to panel-lvds.c.
> The kernel subdir tools/edid has some code to generate the EEPROM data
> from timings and flags.
>
> We keep the DDC EEPROM on a small adapter glued to to back of the panel
> so we can replace the usually short-lived panel with a successor.
OK
Hi Marek,
On 07.12.2024 12:30, Marek Vasut wrote:
> On 12/4/24 11:55 AM, Nikolaus Voss wrote:
>>>>>>>> I doubt that pixel clock tree cannot find appropriate division
>>>>>>>> ratios
>>>>>>>> for some pixel clock rates, especially for dual-link LVDS on
>>>>>>>> i.MX8MP
>>>>>>>> and i.MX93 platforms, because PLL clock rate should be 7x faster
>>>>>>>> than
>>>>>>>> pixel clock rate and 2x faster than "ldb" clock rate so that the
>>>>>>>> 3.5
>>>>>>>> folder between "ldb" clock and pixel clock can be met. That
>>>>>>>> means the
>>>>>>>> PLL clock rate needs to be explicitly set first for this case.
>>>>>>>>
>>>>>>>> Can you assign the PLL clock rate in DT to satisfy the "ldb" and
>>>>>>>> pixel
>>>>>>>> clock rates like the below commit does, if you use a LVDS panel?
>>>>>>>>
>>>>>>>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video
>>>>>>>> PLL1
>>>>>>>> frequency to 506.8 MHz")
>>>>>>>
>>>>>>> I probably could. The point of my patch is you don't have to know
>>>>>>> in
>>>>>>> advance which LVDS panel is connected, and you don't have to
>>>>>>> calculate
>>>>>>> the base PLL clock by hand and store it in the device tree.
>>>>>>>
>>>>>>> In my test system, I have three different LVDS panels with EDID
>>>>>>> EEPROM,
>>>>>>> none of which worked with the stock driver, but all work with
>>>>>>> this
>>>>>>> patch.
>>>>>>> With slightly adapted pixel clocks though.
>>>>>>
>>>>>> If each of the three LVDS panels has only one display mode, you
>>>>>> may
>>>>>> assign the PLL clock rates in DT overlays for the panels.
>>>>> I temporarily agree.
>>>>>
>>>>> I also currently use DTOs for various panels including their PLL
>>>>> setting, but in the end, I think/hope the work of Miquel and co. is
>>>>> going to make that PLL setting part unnecessary.
>>>>
>>>> That is exactly what my patch is about. I want to use one DT for all
>>>> panels
>>>
>>> Right
>>>
>>>> and store the panel's timing in EDID EEPROM.
>>> Oh, that is a new one. Does the EDID EEPROM store the entirety of
>>> 'struct display_timing {}' somehow , or is that a custom format ?
>>
>> Well, sort of ;-). VESA has taken care of this 30 years ago
>> (https://en.wikipedia.org/wiki/Extended_Display_Identification_Data).
>>
>> DRM handles this with drm_get_edid() and siblings, e.g. :
>
> EDID can not encode all the information in struct display_timing {} ,
> or can it ?
>
> I think what you would be missing are bus_flags , bus_format and
> possibly the single/dual link and channel (odd/even) mapping, won't
> you ?
Yes, that's right. I use the vendor block for bus_flags and bus_format
now, but that's not standard and not portable of course.
My first idea was to store the DT overlay in the display EEPROM but
a standard 1k EEPROM is too small for that.
--
Nikolaus Voss
On 12/9/24 9:46 AM, Nikolaus Voss wrote:
Hi,
>>>>> and store the panel's timing in EDID EEPROM.
>>>> Oh, that is a new one. Does the EDID EEPROM store the entirety of
>>>> 'struct display_timing {}' somehow , or is that a custom format ?
>>>
>>> Well, sort of ;-). VESA has taken care of this 30 years ago
>>> (https://en.wikipedia.org/wiki/Extended_Display_Identification_Data).
>>>
>>> DRM handles this with drm_get_edid() and siblings, e.g. :
>>
>> EDID can not encode all the information in struct display_timing {} ,
>> or can it ?
>>
>> I think what you would be missing are bus_flags , bus_format and
>> possibly the single/dual link and channel (odd/even) mapping, won't
>> you ?
>
> Yes, that's right. I use the vendor block for bus_flags and bus_format
> now, but that's not standard and not portable of course.
>
> My first idea was to store the DT overlay in the display EEPROM but
> a standard 1k EEPROM is too small for that.
Understood. I had the same problem, in the end I went for custom
encoding in the EEPROM but the amount of panels was limited in my case.
Indeed, DTO does not fit the EEPROM and EDID is not really fitting too
well to DPI/LVDS panels, it has too many fields that are specific to
regular pluggable panels and not useful on DPI/LVDS ones.
Hi Marek,
On 09.12.2024 22:46, Marek Vasut wrote:
> On 12/9/24 9:46 AM, Nikolaus Voss wrote:
>>>>>> and store the panel's timing in EDID EEPROM.
>>>>> Oh, that is a new one. Does the EDID EEPROM store the entirety of
>>>>> 'struct display_timing {}' somehow , or is that a custom format ?
>>>>
>>>> Well, sort of ;-). VESA has taken care of this 30 years ago
>>>> (https://en.wikipedia.org/wiki/Extended_Display_Identification_Data).
>>>>
>>>> DRM handles this with drm_get_edid() and siblings, e.g. :
>>>
>>> EDID can not encode all the information in struct display_timing {} ,
>>> or can it ?
>>>
>>> I think what you would be missing are bus_flags , bus_format and
>>> possibly the single/dual link and channel (odd/even) mapping, won't
>>> you ?
>>
>> Yes, that's right. I use the vendor block for bus_flags and bus_format
>> now, but that's not standard and not portable of course.
>>
>> My first idea was to store the DT overlay in the display EEPROM but
>> a standard 1k EEPROM is too small for that.
> Understood. I had the same problem, in the end I went for custom
> encoding in the EEPROM but the amount of panels was limited in my
> case.
>
> Indeed, DTO does not fit the EEPROM and EDID is not really fitting too
> well to DPI/LVDS panels, it has too many fields that are specific to
> regular pluggable panels and not useful on DPI/LVDS ones.
The curse of backward-compatibility ;-). Nevertheless, we are doing well
with EDID for years now. 95% is implemented in standard kernel, you just
need a few quirks.
--
Nikolaus Voss
On Tue, Nov 26, 2024 at 04:45:54PM +0100, Nikolaus Voss wrote:
> LDB clock has to be a fixed multiple of the pixel clock.
> As LDB and pixel clock are derived from different clock sources
> (at least on imx8mp), this constraint cannot be satisfied for
> any pixel clock, which leads to flickering and incomplete
> lines on the attached display.
>
> To overcome this, check this condition in mode_fixup() and
> adapt the pixel clock accordingly.
>
> Cc: <stable@vger.kernel.org>
>
> Signed-off-by: Nikolaus Voss <nv@vosn.de>
> ---
> drivers/gpu/drm/bridge/fsl-ldb.c | 40 ++++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 0e4bac7dd04ff..e341341b8c600 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
> return container_of(bridge, struct fsl_ldb, bridge);
> }
>
> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb *fsl_ldb)
> +{
> + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
> +}
> +
> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
> {
> - if (fsl_ldb_is_dual(fsl_ldb))
> - return clock * 3500;
> - else
> - return clock * 7000;
> + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
> }
>
> static int fsl_ldb_attach(struct drm_bridge *bridge,
> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
> bridge, flags);
> }
>
> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
The driver uses atomic callbacks. Please use .atomic_check() instead.
> +{
> + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> + unsigned long requested_link_freq =
> + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
> + unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq);
> +
> + if (freq != requested_link_freq) {
> + /*
> + * this will lead to flicker and incomplete lines on
> + * the attached display, adjust the CRTC clock
> + * accordingly.
> + */
> + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
> +
> + if (adjusted_mode->clock != pclk) {
> + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n",
> + adjusted_mode->clock, pclk);
> +
> + adjusted_mode->clock = pclk;
> + adjusted_mode->crtc_clock = pclk;
> + }
> + }
> +
> + return true;
> +}
> +
> static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_bridge_state)
> {
> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>
> static const struct drm_bridge_funcs funcs = {
> .attach = fsl_ldb_attach,
> + .mode_fixup = fsl_ldb_mode_fixup,
> .atomic_enable = fsl_ldb_atomic_enable,
> .atomic_disable = fsl_ldb_atomic_disable,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> --
> 2.43.0
>
--
With best wishes
Dmitry
Hi Dmitry,
On Sat, 30 Nov 2024, Dmitry Baryshkov wrote:
> On Tue, Nov 26, 2024 at 04:45:54PM +0100, Nikolaus Voss wrote:
>> LDB clock has to be a fixed multiple of the pixel clock.
>> As LDB and pixel clock are derived from different clock sources
>> (at least on imx8mp), this constraint cannot be satisfied for
>> any pixel clock, which leads to flickering and incomplete
>> lines on the attached display.
>>
>> To overcome this, check this condition in mode_fixup() and
>> adapt the pixel clock accordingly.
>>
>> Cc: <stable@vger.kernel.org>
>>
>> Signed-off-by: Nikolaus Voss <nv@vosn.de>
>> ---
>> drivers/gpu/drm/bridge/fsl-ldb.c | 40 ++++++++++++++++++++++++++++----
>> 1 file changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
>> index 0e4bac7dd04ff..e341341b8c600 100644
>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>> @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
>> return container_of(bridge, struct fsl_ldb, bridge);
>> }
>>
>> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb *fsl_ldb)
>> +{
>> + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
>> +}
>> +
>> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
>> {
>> - if (fsl_ldb_is_dual(fsl_ldb))
>> - return clock * 3500;
>> - else
>> - return clock * 7000;
>> + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
>> }
>>
>> static int fsl_ldb_attach(struct drm_bridge *bridge,
>> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
>> bridge, flags);
>> }
>>
>> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
>> + const struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode)
>
> The driver uses atomic callbacks. Please use .atomic_check() instead.
So it is okay to modify drm_crtc_state->adjusted_mode in .atomic_check()?
I chose .mode_fixup() because the function name and args make it more
obvious what is done there. Btw, the API reference doesn't say this call
is deprecated.
A second thought:
Maybe it would be a good idea to reject modes which result in an adjusted
mode pclk that is not within certain boundaries, even if this patch
doesn't do it yet. As I see it, that would be only possible in
mode_fixup().
>
>> +{
>> + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>> + unsigned long requested_link_freq =
>> + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
>> + unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq);
>> +
>> + if (freq != requested_link_freq) {
>> + /*
>> + * this will lead to flicker and incomplete lines on
>> + * the attached display, adjust the CRTC clock
>> + * accordingly.
>> + */
>> + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
>> +
>> + if (adjusted_mode->clock != pclk) {
>> + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n",
>> + adjusted_mode->clock, pclk);
>> +
>> + adjusted_mode->clock = pclk;
>> + adjusted_mode->crtc_clock = pclk;
>> + }
>> + }
>> +
>> + return true;
>> +}
>> +
>> static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>> struct drm_bridge_state *old_bridge_state)
>> {
>> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>>
>> static const struct drm_bridge_funcs funcs = {
>> .attach = fsl_ldb_attach,
>> + .mode_fixup = fsl_ldb_mode_fixup,
>> .atomic_enable = fsl_ldb_atomic_enable,
>> .atomic_disable = fsl_ldb_atomic_disable,
>> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>> --
>> 2.43.0
>>
>
>
--
Nikolaus Voss
On Sat, Nov 30, 2024 at 07:57:17PM +0100, Nikolaus Voss wrote:
> Hi Dmitry,
>
> On Sat, 30 Nov 2024, Dmitry Baryshkov wrote:
> > On Tue, Nov 26, 2024 at 04:45:54PM +0100, Nikolaus Voss wrote:
> > > LDB clock has to be a fixed multiple of the pixel clock.
> > > As LDB and pixel clock are derived from different clock sources
> > > (at least on imx8mp), this constraint cannot be satisfied for
> > > any pixel clock, which leads to flickering and incomplete
> > > lines on the attached display.
> > >
> > > To overcome this, check this condition in mode_fixup() and
> > > adapt the pixel clock accordingly.
> > >
> > > Cc: <stable@vger.kernel.org>
> > >
> > > Signed-off-by: Nikolaus Voss <nv@vosn.de>
> > > ---
> > > drivers/gpu/drm/bridge/fsl-ldb.c | 40 ++++++++++++++++++++++++++++----
> > > 1 file changed, 36 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> > > index 0e4bac7dd04ff..e341341b8c600 100644
> > > --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> > > +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> > > @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
> > > return container_of(bridge, struct fsl_ldb, bridge);
> > > }
> > >
> > > +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb *fsl_ldb)
> > > +{
> > > + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
> > > +}
> > > +
> > > static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
> > > {
> > > - if (fsl_ldb_is_dual(fsl_ldb))
> > > - return clock * 3500;
> > > - else
> > > - return clock * 7000;
> > > + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
> > > }
> > >
> > > static int fsl_ldb_attach(struct drm_bridge *bridge,
> > > @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
> > > bridge, flags);
> > > }
> > >
> > > +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
> > > + const struct drm_display_mode *mode,
> > > + struct drm_display_mode *adjusted_mode)
> >
> > The driver uses atomic callbacks. Please use .atomic_check() instead.
>
> So it is okay to modify drm_crtc_state->adjusted_mode in .atomic_check()? I
Yes. samsung-dsim, anx7625 do that (I stopped checking after the second
one).
> chose .mode_fixup() because the function name and args make it more obvious
> what is done there. Btw, the API reference doesn't say this call is
> deprecated.
It's not deprecated. But as the driver is using atomic calls (vs legacy
calls) it makes more sense to use atomic_check() too.
> A second thought:
> Maybe it would be a good idea to reject modes which result in an adjusted
> mode pclk that is not within certain boundaries, even if this patch doesn't
> do it yet. As I see it, that would be only possible in mode_fixup().
atomic_check() can definitely reject whatever is being stuffed to it.
>
> >
> > > +{
> > > + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> > > + unsigned long requested_link_freq =
> > > + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
> > > + unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq);
> > > +
> > > + if (freq != requested_link_freq) {
> > > + /*
> > > + * this will lead to flicker and incomplete lines on
> > > + * the attached display, adjust the CRTC clock
> > > + * accordingly.
> > > + */
> > > + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
> > > +
> > > + if (adjusted_mode->clock != pclk) {
> > > + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n",
> > > + adjusted_mode->clock, pclk);
> > > +
> > > + adjusted_mode->clock = pclk;
> > > + adjusted_mode->crtc_clock = pclk;
> > > + }
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
> > > struct drm_bridge_state *old_bridge_state)
> > > {
> > > @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
> > >
> > > static const struct drm_bridge_funcs funcs = {
> > > .attach = fsl_ldb_attach,
> > > + .mode_fixup = fsl_ldb_mode_fixup,
> > > .atomic_enable = fsl_ldb_atomic_enable,
> > > .atomic_disable = fsl_ldb_atomic_disable,
> > > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > > --
> > > 2.43.0
> > >
> >
> >
>
> --
> Nikolaus Voss
>
--
With best wishes
Dmitry
+Cc: Miquèl, who is actively working on imx8mp video clock rates.
On Tue, 26 Nov 2024 16:45:54 +0100
Nikolaus Voss <nv@vosn.de> wrote:
> LDB clock has to be a fixed multiple of the pixel clock.
> As LDB and pixel clock are derived from different clock sources
> (at least on imx8mp), this constraint cannot be satisfied for
> any pixel clock, which leads to flickering and incomplete
> lines on the attached display.
>
> To overcome this, check this condition in mode_fixup() and
> adapt the pixel clock accordingly.
>
> Cc: <stable@vger.kernel.org>
>
> Signed-off-by: Nikolaus Voss <nv@vosn.de>
> ---
> drivers/gpu/drm/bridge/fsl-ldb.c | 40 ++++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 0e4bac7dd04ff..e341341b8c600 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
> return container_of(bridge, struct fsl_ldb, bridge);
> }
>
> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb *fsl_ldb)
> +{
> + return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
> +}
> +
> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
> {
> - if (fsl_ldb_is_dual(fsl_ldb))
> - return clock * 3500;
> - else
> - return clock * 7000;
> + return clock * fsl_ldb_link_freq_factor(fsl_ldb);
> }
>
> static int fsl_ldb_attach(struct drm_bridge *bridge,
> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
> bridge, flags);
> }
>
> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> +{
> + const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> + unsigned long requested_link_freq =
> + mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
> + unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq);
> +
> + if (freq != requested_link_freq) {
> + /*
> + * this will lead to flicker and incomplete lines on
> + * the attached display, adjust the CRTC clock
> + * accordingly.
> + */
> + int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);
> +
> + if (adjusted_mode->clock != pclk) {
> + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n",
> + adjusted_mode->clock, pclk);
> +
> + adjusted_mode->clock = pclk;
> + adjusted_mode->crtc_clock = pclk;
> + }
> + }
> +
> + return true;
> +}
> +
> static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_bridge_state)
> {
> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>
> static const struct drm_bridge_funcs funcs = {
> .attach = fsl_ldb_attach,
> + .mode_fixup = fsl_ldb_mode_fixup,
> .atomic_enable = fsl_ldb_atomic_enable,
> .atomic_disable = fsl_ldb_atomic_disable,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
© 2016 - 2026 Red Hat, Inc.