On Mon, 07 Oct 2019, "Jani Nikula" <jani.nik...@intel.com<mailto:jani.nik...@intel.com>> wrote: >On Mon, 07 Oct 2019, "Lee, Shawn C" ><shawn.c....@intel.com<mailto:shawn.c....@intel.com>> wrote: >> On Fri, 04 Oct 2019, Jani Nikula >> <jani.nik...@intel.com<mailto:jani.nik...@intel.com>> wrote: >>>On Fri, 04 Oct 2019, Adam Jackson <a...@redhat.com<mailto:a...@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 >>>
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
