<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72"> <div class="WordSection1"> <o:p> </o:p> On Mon, 07 Oct 2019, "Jani Nikula" <<a href="mailto:jani.nikula@intel.com">jani.nikula@intel.com</a>> wrote:<o:p></o:p> >On Mon, 07 Oct 2019, "Lee, Shawn C" <<a href="mailto:shawn.c.lee@intel.com">shawn.c.lee@intel.com</a>> wrote:<o:p></o:p> >> On Fri, 04 Oct 2019, Jani Nikula <<a href="mailto:jani.nikula@intel.com">jani.nikula@intel.com</a>> wrote:<o:p></o:p> >>>On Fri, 04 Oct 2019, Adam Jackson <<a href="mailto:ajax@redhat.com">ajax@redhat.com</a>> wrote:<o:p></o:p> >>>> On Sat, 2019-10-05 at 05:58 +0800, Lee Shawn C wrote:<o:p></o:p> >>>>> This panel (manufacturer is SDC, product ID is 0x4141) used <o:p></o:p> >>>>> manufacturer defined DPCD register to control brightness that not <o:p></o:p> >>>>> defined in eDP spec so far. This change follow panel vendor's <o:p></o:p> >>>>> instruction to support brightness adjustment.<o:p></o:p> >>>><o:p> </o:p> >>>> I'm sure this works, but this smells a little funny to me.<o:p></o:p> >>><o:p> </o:p> >>>That was kindly put. ;)<o:p></o:p> >>><o:p> </o:p> >>>>> + /* Samsung eDP panel */<o:p></o:p> >>>>> + { "SDC", 0x4141, EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL },<o:p></o:p> >>>><o:p> </o:p> >>>> It feels a bit like a layering violation to identify eDP behavior <o:p></o:p> >>>> changes based on EDID. But I'm not sure there's any obvious way to <o:p></o:p> >>>> identify this device by its DPCD. The Sink OUI (from the linked<o:p></o:p> >>>> bugzilla) seems to be 0011F8, which doesn't match up to anything in my <o:p></o:p> >>>> oui.txt...<o:p></o:p> >>><o:p> </o:p> >>>We have the DPCD quirk stuff in drm_dp_helper.c, but IIUC in this case <o:p></o:p> >>>there's only the OUI, and the device id etc. are all zeros. Otherwise I<o:p></o:p> >>>think that would be the natural thing to do, and all this could be<o:p></o:p> >>>better hidden away in i915.<o:p></o:p> >>><o:p> </o:p> >><o:p> </o:p> >> Below is what we dumped from this panel. Only sink OUI (ba-41-59) in it<o:p></o:p> >> and nothing else. <o:p></o:p> >> 00000400 ba 41 59 00 00 00 00 00 00 00 00 00 00 00 00 00 |.AY.............|<o:p></o:p> >> 00000410 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................<o:p></o:p> >><o:p> </o:p> >> That's why the patch to identify EDID's manufacturer and product ID<o:p></o:p> >> to make sure this method applied on specific panel.<o:p></o:p> >><o:p> </o:p> >>>><o:p> </o:p> >>>>> @@ -1953,6 +1956,7 @@ static u32 edid_get_quirks(const struct edid <o:p></o:p> >>>>> *edid)<o:p></o:p> >>>>> <o:p></o:p> >>>>> return 0;<o:p></o:p> >>>>> }<o:p></o:p> >>>>> +EXPORT_SYMBOL(edid_get_quirks);<o:p></o:p> >>>><o:p> </o:p> >>>> If we're going to export this it should probably get a drm_ prefix.<o:p></o:p> >><o:p> </o:p> >> Yes! It will be better to have drm_ prefix for export funciton.<o:p></o:p> >><o:p> </o:p> >>>><o:p> </o:p> >>>>> +#define DPCD_EDP_GETSET_CTRL_PARAMS 0x344<o:p></o:p> >>>>> +#define DPCD_EDP_CONTENT_LUMINANCE 0x346<o:p></o:p> >>>>> +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE 0x34a<o:p></o:p> >>>>> +#define DPCD_EDP_BRIGHTNESS_NITS 0x354<o:p></o:p> >>>>> +#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION 0x358<o:p></o:p> >>>>> +<o:p></o:p> >>>>> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL (512)<o:p></o:p> >>>><o:p> </o:p> >>>> This also seems a bit weird, the 0x300-0x3FF registers belong to the <o:p></o:p> >>>> _source_ DP device. But then later...<o:p></o:p> >>>><o:p> </o:p> >>>>> + /* write source OUI */<o:p></o:p> >>>>> + write_val[0] = 0x00;<o:p></o:p> >>>>> + write_val[1] = 0xaa;<o:p></o:p> >>>>> + write_val[2] = 0x01;<o:p></o:p> >>>><o:p> </o:p> >>>> Oh hey, you're writing (an) Intel OUI to the Source OUI, so now it <o:p></o:p> >>>> makes sense that you're writing to registers whose behavior the source <o:p></o:p> >>>> defines. But this does raise the question: is this just a convention <o:p></o:p> >>>> between Intel and this particular panel? Would we expect this to work <o:p></o:p> >>>> with other similar panels? Is there any way to know to expect this <o:p></o:p> >>>> convention from DPCD instead?<o:p></o:p> >><o:p> </o:p> >> TCON would reply on source OUI to configure its capability. And these<o:p></o:p> >> DPCD registers were defined by vendor and Intel. This change should works<o:p></o:p> >> with similar panels (with same TCON). Seems there is another issue so<o:p></o:p> >> vendor decide to use non standard way to setup brightness.<o:p></o:p> >><o:p> </o:p> >>>For one thing, it's not standard. I honestly don't know, but I'd assume<o:p></o:p> >>>you wouldn't find behaviour with Intel OUI in non-Intel designs... and a<o:p></o:p> >>>quirk of some sort seems like the only way to make this work.<o:p></o:p> >>><o:p> </o:p> >>>I suppose we could start off with a DPCD quirk that only looks at the<o:p></o:p> >>>sink OUI, and then, if needed, limit by DMI matching or by checking for<o:p></o:p> >>>some DPCD registers (what, I am not sure, perhaps write the source OUI<o:p></o:p> >>>and see how it behaves).<o:p></o:p> >>><o:p> </o:p> >>>That would avoid the mildly annoying change in the EDID quirk interface<o:p></o:p> >>>and how it's being used.<o:p></o:p> >>><o:p> </o:p> >>>Thoughts?<o:p></o:p> >>><o:p> </o:p> >>><o:p> </o:p> >>>BR,<o:p></o:p> >>>Jani.<o:p></o:p> >>><o:p> </o:p> >><o:p> </o:p> >> To be honest. Panel vendor did not provide enough sink info in DPCD.<o:p></o:p> >> That's why hard to recognize it and we have to confirm EDID instead of DPCD.<o:p></o:p> >><o:p> </o:p> >> Do you mean just confirm sink OUI only from DPCD quirk? I'm afraid it<o:p></o:p> >> may impact the other panels with the same TCON. Any suggestion?<o:p></o:p> ><o:p> </o:p> >The problem with the EDID quirks is that exposing the quirks sticks out<o:p></o:p> >like a sore thumb. Thus far all of it has been contained in drm_edid.c<o:p></o:p> >and they affect how the EDID gets parsed, for all drivers. Obviously<o:p></o:p> >this could be changed, but is it the right thing to do?<o:p></o:p> ><o:p> </o:p> >What I suggested was, check the OUI only, and if it matches, do<o:p></o:p> >more. Perhaps there's something in the 0x300 range of DPCD offsets that<o:p></o:p> >you can read? Or perhaps you need to write the source OUI first, and<o:p></o:p> >then do that.<o:p></o:p> ><o:p> </o:p> >BR,<o:p></o:p> >Jani.<o:p></o:p> ><o:p> </o:p> <o:p> </o:p> These bytes are RO. Seems we can used it to identify this panel<o:p></o:p> as well. I will use DPCD quirk and renew this patch again.<o:p></o:p> <o:p> </o:p> ><o:p> </o:p> >><o:p> </o:p> >>><o:p> </o:p> >>>--<o:p></o:p> >>>Jani Nikula, Intel Open Source Graphics Center<o:p></o:p> >>><o:p> </o:p> <o:p> </o:p> </div> </body> </html>