| 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 |
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); >
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);
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
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 --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);
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(-)