On Tue, 08 Oct 2019, Jani Nikula jani.nikula@intel.com wrote:
On Mon, 07 Oct 2019, Adam Jackson ajax@redhat.com wrote:
On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
The problem with the EDID quirks is that exposing the quirks sticks out like a sore thumb. Thus far all of it has been contained in drm_edid.c and they affect how the EDID gets parsed, for all drivers. Obviously this could be changed, but is it the right thing to do?
What I suggested was, check the OUI only, and if it matches, do more. Perhaps there's something in the 0x300 range of DPCD offsets that you can read? Or perhaps you need to write the source OUI first, and then do that.
My issue isn't really with identifying the panel from EDID rather than DPCD, whichever identifier is most specific is probably the best thing to use. It's more that this quirk is identified in common code but only applied in one driver. If this panel were ever to be attached to some other source, they might well want to apply the same kind of fix. My (admittedly naïve) reading of the OUI handshake process is that when the source device writes an OUI to DP_SOURCE_OUI it is telling the sink "I'm about to issue commands that conform to _this_ vendor's own conventions". If that convention communicates information that is entirely contained within AUXCH transactions (and doesn't, for example, require looking at some other strapping pin or external device) then in principle it doesn't matter if the source device "matches" that OUI; it would be legal for an AMD GPU to write the same sequence and expect the same reaction, should that panel be attached to an AMD GPU.
So, it would be nice to know exactly what that protocol is meant to do, if it applies only to this specific panel or anything else with the same TCON, how one would identify such TCONs in the wild other than EDID, if it relies on an external PWM or something, etc. And it might make sense for now to make this a (shudder) driver-specific EDID quirk rather than match by DPCD, at least until we know if the panel is ever seen attached to other source devices and if the OUI convention is self-contained.
Thanks for clarifying. Pretty much agreed, unfortunately also on the "would be nice to know more" part...
If this were to be an EDID quirk after all, I wonder if it would be better to store the parsed quirks to, say, struct drm_display_info, and have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().
This would also allow us to not return quirks from drm_add_display_info(), which would arguably clean up the interface.
BR, Jani.
Thanks for advice! I've already update patch V2. Driver will check sink OUI and confirm TCON's capability to decide to enable this method or not. It depends on TCON's feature description and does not export EDID quirk.
Best regards, Shawn