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

[Lee, Shawn C]

On Fri, 04 Oct 2019, Jani Nikula <jani.nikula at intel.com> wrote:
>On Fri, 04 Oct 2019, Adam Jackson <ajax at redhat.com> wrote:
>> 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.
>
>That was kindly put. ;)
>
>>> +	/* 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...
>
>We have the DPCD quirk stuff in drm_dp_helper.c, but IIUC in this case 
>there's only the OUI, and the device id etc. are all zeros. Otherwise I
>think that would be the natural thing to do, and all this could be
>better hidden away in i915.
>

Below is what we dumped from this panel. Only sink OUI (ba-41-59) in it
and nothing else. 
00000400  ba 41 59 00 00 00 00 00  00 00 00 00 00 00 00 00  |.AY.............|
00000410  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................

That's why the patch to identify EDID's manufacturer and product ID
to make sure this method applied on specific panel.

>>
>>> @@ -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.

Yes! It will be better to have drm_ prefix for export funciton.

>>
>>> +#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?

TCON would reply on source OUI to configure its capability. And these
DPCD registers were defined by vendor and Intel. This change should works
with similar panels (with same TCON). Seems there is another issue so
vendor decide to use non standard way to setup brightness.

>For one thing, it's not standard. I honestly don't know, but I'd assume
>you wouldn't find behaviour with Intel OUI in non-Intel designs... and a
>quirk of some sort seems like the only way to make this work.
>
>I suppose we could start off with a DPCD quirk that only looks at the
>sink OUI, and then, if needed, limit by DMI matching or by checking for
>some DPCD registers (what, I am not sure, perhaps write the source OUI
>and see how it behaves).
>
>That would avoid the mildly annoying change in the EDID quirk interface
>and how it's being used.
>
>Thoughts?
>
>
>BR,
>Jani.
>

To be honest. Panel vendor did not provide enough sink info in DPCD.
That's why hard to recognize it and we have to confirm EDID instead of DPCD.

Do you mean just confirm sink OUI only from DPCD quirk? I'm afraid it
may impact the other panels with the same TCON. Any suggestion?

>
>--
>Jani Nikula, Intel Open Source Graphics Center
>