[Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Jani Nikula]

On Tue, 26 Nov 2019, Lyude Paul <lyude at redhat.com> wrote:
> I'm about to post some more review comments for the v2 version of this, but
> some comments down below...
>
> On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote:
>> On Mon, 07 Oct 2019, Adam Jackson <ajax at 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.
>
> Did anyone check if this is specified in the vbios? There appears to be a
> field defined for this right...
>
> enum intel_backlight_type {
> 	INTEL_BACKLIGHT_PMIC,
> 	INTEL_BACKLIGHT_LPSS,
> 	INTEL_BACKLIGHT_DISPLAY_DDI,
> 	INTEL_BACKLIGHT_DSI_DCS,
> 	INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */
> 	INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE,
> };

Would just need /sys/kernel/debug/dri/0/i915_vbt on the affected machine
to check.

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center