Re: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Jani Nikula <jani.nikula@intel.com>]

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.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx