diff mbox series

drm/bridge: tc358767: fix max_tu_symbol value

Message ID 20190924131702.9988-1-tomi.valkeinen@ti.com
State Accepted
Commit fd70c7755bf0172ddd33b558aef69c322de3b5cf
Headers show
Series drm/bridge: tc358767: fix max_tu_symbol value | expand

Commit Message

Tomi Valkeinen Sept. 24, 2019, 1:17 p.m. UTC
max_tu_symbol was programmed to TU_SIZE_RECOMMENDED - 1, which is not
what the spec says. The spec says:

roundup ((input active video bandwidth in bytes/output active video
bandwidth in bytes) * tu_size)

It is not quite clear what the above means, but calculating
max_tu_symbol = (input Bps / output Bps) * tu_size seems to work and
fixes the issues seen.

This fixes artifacts in some videomodes (e.g. 1024x768@60 on 2-lanes &
1.62Gbps was pretty bad for me).

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Jyri Sarha Sept. 25, 2019, 7:37 a.m. UTC | #1
On 24/09/2019 16:17, Tomi Valkeinen wrote:
> max_tu_symbol was programmed to TU_SIZE_RECOMMENDED - 1, which is not
> what the spec says. The spec says:
> 
> roundup ((input active video bandwidth in bytes/output active video
> bandwidth in bytes) * tu_size)
> 
> It is not quite clear what the above means, but calculating
> max_tu_symbol = (input Bps / output Bps) * tu_size seems to work and
> fixes the issues seen.
> 
> This fixes artifacts in some videomodes (e.g. 1024x768@60 on 2-lanes &
> 1.62Gbps was pretty bad for me).
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I could reproduce the problem and see that the patch fixes it, so:

Tested-by: Jyri Sarha <jsarha@ti.com>


> ---
>  drivers/gpu/drm/bridge/tc358767.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 13ade28a36a8..b6aa1bd47e1d 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -677,6 +677,8 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	int upper_margin = mode->vtotal - mode->vsync_end;
>  	int lower_margin = mode->vsync_start - mode->vdisplay;
>  	int vsync_len = mode->vsync_end - mode->vsync_start;
> +	u32 bits_per_pixel = 24;
> +	u32 in_bw, out_bw;
>  
>  	/*
>  	 * Recommended maximum number of symbols transferred in a transfer unit:
> @@ -684,7 +686,10 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	 *              (output active video bandwidth in bytes))
>  	 * Must be less than tu_size.
>  	 */
> -	max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
> +
> +	in_bw = mode->clock * bits_per_pixel / 8;
> +	out_bw = tc->link.base.num_lanes * tc->link.base.rate;
> +	max_tu_symbol = DIV_ROUND_UP(in_bw * TU_SIZE_RECOMMENDED, out_bw);
>  
>  	dev_dbg(tc->dev, "set mode %dx%d\n",
>  		mode->hdisplay, mode->vdisplay);
>
Andrzej Hajda Oct. 10, 2019, 9:19 a.m. UTC | #2
On 24.09.2019 15:17, Tomi Valkeinen wrote:
> max_tu_symbol was programmed to TU_SIZE_RECOMMENDED - 1, which is not
> what the spec says. The spec says:
>
> roundup ((input active video bandwidth in bytes/output active video
> bandwidth in bytes) * tu_size)
>
> It is not quite clear what the above means, but calculating
> max_tu_symbol = (input Bps / output Bps) * tu_size seems to work and
> fixes the issues seen.
>
> This fixes artifacts in some videomodes (e.g. 1024x768@60 on 2-lanes &
> 1.62Gbps was pretty bad for me).
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>


Queued to fixes.


Regards

Andrzej


> ---
>  drivers/gpu/drm/bridge/tc358767.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 13ade28a36a8..b6aa1bd47e1d 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -677,6 +677,8 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	int upper_margin = mode->vtotal - mode->vsync_end;
>  	int lower_margin = mode->vsync_start - mode->vdisplay;
>  	int vsync_len = mode->vsync_end - mode->vsync_start;
> +	u32 bits_per_pixel = 24;
> +	u32 in_bw, out_bw;
>  
>  	/*
>  	 * Recommended maximum number of symbols transferred in a transfer unit:
> @@ -684,7 +686,10 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	 *              (output active video bandwidth in bytes))
>  	 * Must be less than tu_size.
>  	 */
> -	max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
> +
> +	in_bw = mode->clock * bits_per_pixel / 8;
> +	out_bw = tc->link.base.num_lanes * tc->link.base.rate;
> +	max_tu_symbol = DIV_ROUND_UP(in_bw * TU_SIZE_RECOMMENDED, out_bw);
>  
>  	dev_dbg(tc->dev, "set mode %dx%d\n",
>  		mode->hdisplay, mode->vdisplay);
Tomi Valkeinen Oct. 10, 2019, 9:25 a.m. UTC | #3
Hi Andrzej,

On 10/10/2019 12:19, Andrzej Hajda wrote:
> On 24.09.2019 15:17, Tomi Valkeinen wrote:
>> max_tu_symbol was programmed to TU_SIZE_RECOMMENDED - 1, which is not
>> what the spec says. The spec says:
>>
>> roundup ((input active video bandwidth in bytes/output active video
>> bandwidth in bytes) * tu_size)
>>
>> It is not quite clear what the above means, but calculating
>> max_tu_symbol = (input Bps / output Bps) * tu_size seems to work and
>> fixes the issues seen.
>>
>> This fixes artifacts in some videomodes (e.g. 1024x768@60 on 2-lanes &
>> 1.62Gbps was pretty bad for me).
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> 
> Queued to fixes.

If you didn't push this yet, can you drop it for now? This works for all 
the video modes I have tested, but as I mention above, the calculation 
is really not quite clear to me.

I've sent queries to Toshiba about how to calculate this, and I'm hoping 
to get a reply soon.

If you did push it already, that's fine too, as it does improve things.

  Tomi
Tomi Valkeinen Nov. 6, 2019, 11:23 a.m. UTC | #4
On 10/10/2019 12:25, Tomi Valkeinen wrote:
> Hi Andrzej,
> 
> On 10/10/2019 12:19, Andrzej Hajda wrote:
>> On 24.09.2019 15:17, Tomi Valkeinen wrote:
>>> max_tu_symbol was programmed to TU_SIZE_RECOMMENDED - 1, which is not
>>> what the spec says. The spec says:
>>>
>>> roundup ((input active video bandwidth in bytes/output active video
>>> bandwidth in bytes) * tu_size)
>>>
>>> It is not quite clear what the above means, but calculating
>>> max_tu_symbol = (input Bps / output Bps) * tu_size seems to work and
>>> fixes the issues seen.
>>>
>>> This fixes artifacts in some videomodes (e.g. 1024x768@60 on 2-lanes &
>>> 1.62Gbps was pretty bad for me).
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>>
>> Queued to fixes.
> 
> If you didn't push this yet, can you drop it for now? This works for all 
> the video modes I have tested, but as I mention above, the calculation 
> is really not quite clear to me.
> 
> I've sent queries to Toshiba about how to calculate this, and I'm hoping 
> to get a reply soon.
> 
> If you did push it already, that's fine too, as it does improve things.
Just for the record, got a reply from Toshiba, and the patch is correct.

  Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 13ade28a36a8..b6aa1bd47e1d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -677,6 +677,8 @@  static int tc_set_video_mode(struct tc_data *tc,
 	int upper_margin = mode->vtotal - mode->vsync_end;
 	int lower_margin = mode->vsync_start - mode->vdisplay;
 	int vsync_len = mode->vsync_end - mode->vsync_start;
+	u32 bits_per_pixel = 24;
+	u32 in_bw, out_bw;
 
 	/*
 	 * Recommended maximum number of symbols transferred in a transfer unit:
@@ -684,7 +686,10 @@  static int tc_set_video_mode(struct tc_data *tc,
 	 *              (output active video bandwidth in bytes))
 	 * Must be less than tu_size.
 	 */
-	max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
+
+	in_bw = mode->clock * bits_per_pixel / 8;
+	out_bw = tc->link.base.num_lanes * tc->link.base.rate;
+	max_tu_symbol = DIV_ROUND_UP(in_bw * TU_SIZE_RECOMMENDED, out_bw);
 
 	dev_dbg(tc->dev, "set mode %dx%d\n",
 		mode->hdisplay, mode->vdisplay);