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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
