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

[Lyude Paul]

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,
};

> 
> BR,
> Jani.
> 
> 
-- 
Cheers,
	Lyude Paul