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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



 

On Mon, 07 Oct 2019, "Jani Nikula" <jani.nikula@xxxxxxxxx> wrote:

>On Mon, 07 Oct 2019, "Lee, Shawn C" <shawn.c.lee@xxxxxxxxx> wrote:

>> On Fri, 04 Oct 2019, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:

>>>On Fri, 04 Oct 2019, Adam Jackson <ajax@xxxxxxxxxx> 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

>>> 

 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux