[PATCH] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Nikolaus Voss]

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 at vger.kernel.org>
>>
>> Signed-off-by: Nikolaus Voss <nv at 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