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

[Lee, Shawn C]

On Mon, 07 Oct 2019, "Jani Nikula" <jani.nikula at intel.com<mailto:jani.nikula at intel.com>> wrote:
>On Mon, 07 Oct 2019, "Lee, Shawn C" <shawn.c.lee at intel.com<mailto:shawn.c.lee at intel.com>> wrote:
>> On Fri, 04 Oct 2019, Jani Nikula <jani.nikula at intel.com<mailto:jani.nikula at intel.com>> wrote:
>>>On Fri, 04 Oct 2019, Adam Jackson <ajax at redhat.com<mailto: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?
>
>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.
>
>BR,
>Jani.
>

These bytes are RO. Seems we can used it to identify this panel
as well. I will use DPCD quirk and renew this patch again.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191007/4e813d74/attachment.html>