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

[Adam Jackson]

On Sat, 2019-10-05 at 05:58 +0800, Lee Shawn C wrote:
> This panel (manufacturer is SDC, product ID is 0x4141)
> used manufacturer defined DPCD register to control brightness
> that not defined in eDP spec so far. This change follow panel
> vendor's instruction to support brightness adjustment.

I'm sure this works, but this smells a little funny to me.

> +	/* Samsung eDP panel */
> +	{ "SDC", 0x4141, EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL },

It feels a bit like a layering violation to identify eDP behavior
changes based on EDID. But I'm not sure there's any obvious way to
identify this device by its DPCD. The Sink OUI (from the linked
bugzilla) seems to be 0011F8, which doesn't match up to anything in my
oui.txt...

> @@ -1953,6 +1956,7 @@ static u32 edid_get_quirks(const struct edid *edid)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(edid_get_quirks);

If we're going to export this it should probably get a drm_ prefix.

> +#define DPCD_EDP_GETSET_CTRL_PARAMS		0x344
> +#define DPCD_EDP_CONTENT_LUMINANCE		0x346
> +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE	0x34a
> +#define DPCD_EDP_BRIGHTNESS_NITS		0x354
> +#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION	0x358
> +
> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)

This also seems a bit weird, the 0x300-0x3FF registers belong to the
_source_ DP device. But then later...

> +	/* write source OUI */
> +	write_val[0] = 0x00;
> +	write_val[1] = 0xaa;
> +	write_val[2] = 0x01;

Oh hey, you're writing (an) Intel OUI to the Source OUI, so now it
makes sense that you're writing to registers whose behavior the source
defines. But this does raise the question: is this just a convention
between Intel and this particular panel? Would we expect this to work
with other similar panels? Is there any way to know to expect this
convention from DPCD instead?

- ajax