[PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Laurent Pinchart]

Hi Tomi,

On Fri, May 03, 2019 at 04:17:41PM +0300, Tomi Valkeinen wrote:
> On 03/05/2019 15:48, Laurent Pinchart wrote:
> > On Fri, May 03, 2019 at 02:43:51PM +0300, Tomi Valkeinen wrote:
> >> On 23/04/2019 17:56, Laurent Pinchart wrote:
> >>
> >>>> During initial driver development I had one eDP display that reports 0 in Bit 0
> >>>> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> >>>> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> >>>> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> >>>> So I had to disable 8B10 encoding on tc358767 side to make this display
> >>>> work.
> >>>
> >>> Out of curiosity, how does the eDP display recover the clock without
> >>> 8B/10B encoding ?
> >>>
> >>>> On other hand if there are displays that report zero bit 0 in
> >>>> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> >>>> reasonable.
> >>>>
> >>>> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> >>>> register after setting it and check if 8b10b actually enabled?
> >>>
> >>> This sounds like a reasonable thing to try. Tomi, do you still have
> >>> accesss to those faulty monitors ?
> >>
> >> On my monitor it does not seem to matter whether I write 0 or 1 to
> >> MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
> >> TC358767 side. The writes do go through, and I can read the written bit
> >> back.
> >>
> >> So... I guess when we talk about eDP panels, there may be all kinds of
> >> oddities, as they're usually meant to be used with a certain configuration.
> >>
> >> But if everyone agrees that 8B10B is a normal, required feature of DP,
> >> and there is an eDP panel that needs 8B10B disabled, maybe that panel
> >> has to be handled separately as a special case? A dts entry to disable
> >> 8B10B? Or something. But it does not sound like a good idea for the
> >> driver to try to guess this.
> > 
> > As reported by Andrey, there is at least one panel that would break with
> > this patch. I agree 8b10b should be the default, but if the above
> > procedure works for all the panels we know about, is there an issue
> > implementing it ?
> 
> Well, we don't have data for a big set of panels. I'm worried that such
> a change, which, in my opinion, makes the driver guess whether to enable
> or disable 8b10b, can break other panels or monitors if the guess
> doesn't go right. Especially as disabling 8b10b does not sound like a
> valid setup "officially".
> 
> I agree that if the panel Andrey mentioned is still used, we need to
> handle it somehow. But I think explicitly handling such a case is better
> than guessing.

The risk may not be worth it, I agree. I would explain this in details
in the commit message though, and add a comment to the code as well, to
ease future debugging.

> And isn't this something that's not really TC358767 specific? If that
> panel is used with other bridges, we need to deal with this case with
> those bridges too? Or is TC358767 the only bridge that allows disabling
> 8b10b?

I don't know about other bridges, but I think it would need to be
handled globally, yes.

-- 
Regards,

Laurent Pinchart