[PATCH] drm/bridge: tc358767: Fix use of unadjusted mode in the driver

[PATCH] drm/bridge: tc358767: Fix use of unadjusted mode in the driver

[Marek Vasut]

The driver configures mostly Pixel PLL from the clock cached in
local copy of the mode. Make sure the driver uses adjusted mode
which contains the updated Pixel PLL settings negotiated in
tc_dpi_atomic_check()/tc_edp_atomic_check().

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7968183510e63..0d523322fdd8e 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1764,7 +1764,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 
-	drm_mode_copy(&tc->mode, mode);
+	drm_mode_copy(&tc->mode, adj);
 }
 
 static const struct drm_edid *tc_edid_read(struct drm_bridge *bridge,
-- 
2.45.2

Re: [PATCH] drm/bridge: tc358767: Fix use of unadjusted mode in the driver

[Robert Foss]

On Sat, 26 Oct 2024 06:10:42 +0200, Marek Vasut wrote:
> The driver configures mostly Pixel PLL from the clock cached in
> local copy of the mode. Make sure the driver uses adjusted mode
> which contains the updated Pixel PLL settings negotiated in
> tc_dpi_atomic_check()/tc_edp_atomic_check().
> 
> 

Applied, thanks!

[1/1] drm/bridge: tc358767: Fix use of unadjusted mode in the driver
      https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/b59d1d9d9ab2



Rob

Re: drm/bridge: tc358767: Fix use of unadjusted mode in the driver

[Alexander Stein]

Hi Marek,

Am Samstag, 26. Oktober 2024, 06:10:42 CET schrieb Marek Vasut:
> The driver configures mostly Pixel PLL from the clock cached in
> local copy of the mode. Make sure the driver uses adjusted mode
> which contains the updated Pixel PLL settings negotiated in
> tc_dpi_atomic_check()/tc_edp_atomic_check().
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 7968183510e63..0d523322fdd8e 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1764,7 +1764,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  
> -	drm_mode_copy(&tc->mode, mode);
> +	drm_mode_copy(&tc->mode, adj);
>  }
>  
>  static const struct drm_edid *tc_edid_read(struct drm_bridge *bridge,
> 

This unfortunately breaks my DSI->DP setup on TQMa8MPxL/MBa8MPxL.

When I revert it, DP works again. I'm currently running on next-20241114
with patches regarding DSI initialization.

before:
tc358767 1-000f: PLL: requested 148500000 pixelclock, ref 26000000
tc358767 1-000f: PLL: got 147333333, delta -1166667
tc358767 1-000f: PLL: 26000000 / 1 / 1 * 17 / 3
tc358767 1-000f: set mode 1920x1080
tc358767 1-000f: H margin 148,88 sync 44
tc358767 1-000f: V margin 36,4 sync 5
tc358767 1-000f: total: 2200x1125

after:
tc358767 1-000f: PLL: requested 147333000 pixelclock, ref 26000000
tc358767 1-000f: PLL: got 146250000, delta -1083000
tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
tc358767 1-000f: set mode 1920x1080
tc358767 1-000f: H margin 148,88 sync 44
tc358767 1-000f: V margin 36,4 sync 5
tc358767 1-000f: total: 2200x1125

The reason this breaks is that the adjusted clock frequency is slightly off
to the previously calculated one: 147333333 <-> 147333000
This is because mode clock is only specyfied in KHz. With this incorrect
input the new result is even lower than requested, thus failing to setup
correctly.

As I don't see a quick fix, I propose a revert meanwhile. Any comments?

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: drm/bridge: tc358767: Fix use of unadjusted mode in the driver

[Marek Vasut]

On 11/14/24 10:27 AM, Alexander Stein wrote:
> Hi Marek,

Hi,

> Am Samstag, 26. Oktober 2024, 06:10:42 CET schrieb Marek Vasut:
>> The driver configures mostly Pixel PLL from the clock cached in
>> local copy of the mode. Make sure the driver uses adjusted mode
>> which contains the updated Pixel PLL settings negotiated in
>> tc_dpi_atomic_check()/tc_edp_atomic_check().
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Cc: Jonas Karlman <jonas@kwiboo.se>
>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>> Cc: Robert Foss <rfoss@kernel.org>
>> Cc: Simona Vetter <simona@ffwll.ch>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/bridge/tc358767.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 7968183510e63..0d523322fdd8e 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1764,7 +1764,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
>>   {
>>   	struct tc_data *tc = bridge_to_tc(bridge);
>>   
>> -	drm_mode_copy(&tc->mode, mode);
>> +	drm_mode_copy(&tc->mode, adj);
>>   }
>>   
>>   static const struct drm_edid *tc_edid_read(struct drm_bridge *bridge,
>>
> 
> This unfortunately breaks my DSI->DP setup on TQMa8MPxL/MBa8MPxL.
> 
> When I revert it, DP works again. I'm currently running on next-20241114
> with patches regarding DSI initialization.
> 
> before:
> tc358767 1-000f: PLL: requested 148500000 pixelclock, ref 26000000
> tc358767 1-000f: PLL: got 147333333, delta -1166667
> tc358767 1-000f: PLL: 26000000 / 1 / 1 * 17 / 3
> tc358767 1-000f: set mode 1920x1080
> tc358767 1-000f: H margin 148,88 sync 44
> tc358767 1-000f: V margin 36,4 sync 5
> tc358767 1-000f: total: 2200x1125
> 
> after:
> tc358767 1-000f: PLL: requested 147333000 pixelclock, ref 26000000
> tc358767 1-000f: PLL: got 146250000, delta -1083000
> tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> tc358767 1-000f: set mode 1920x1080
> tc358767 1-000f: H margin 148,88 sync 44
> tc358767 1-000f: V margin 36,4 sync 5
> tc358767 1-000f: total: 2200x1125
> 
> The reason this breaks is that the adjusted clock frequency is slightly off
> to the previously calculated one: 147333333 <-> 147333000
> This is because mode clock is only specyfied in KHz. With this incorrect
> input the new result is even lower than requested, thus failing to setup
> correctly.
> 
> As I don't see a quick fix, I propose a revert meanwhile. Any comments?
Sigh, the TC358767 is really a gift that keeps on giving.

Either revert it for now, or give me a week or three until I get back to 
digging in the TC358767. Whichever you prefer. Sorry for the slowness.