Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Tue, 26 Nov 2019, Lyude Paul wrote: > I'm about to post some more review comments for the v2 version of this, but > some comments down below... > > On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote: >> On Mon, 07 Oct 2019, Adam Jackson wrote: >> > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote: >> > >> > > 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. >> > >> > My issue isn't really with identifying the panel from EDID rather than >> > DPCD, whichever identifier is most specific is probably the best thing >> > to use. It's more that this quirk is identified in common code but only >> > applied in one driver. If this panel were ever to be attached to some >> > other source, they might well want to apply the same kind of fix. My >> > (admittedly naïve) reading of the OUI handshake process is that when >> > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink >> > "I'm about to issue commands that conform to _this_ vendor's own >> > conventions". If that convention communicates information that is >> > entirely contained within AUXCH transactions (and doesn't, for example, >> > require looking at some other strapping pin or external device) then in >> > principle it doesn't matter if the source device "matches" that OUI; it >> > would be legal for an AMD GPU to write the same sequence and expect the >> > same reaction, should that panel be attached to an AMD GPU. >> > >> > So, it would be nice to know exactly what that protocol is meant to do, >> > if it applies only to this specific panel or anything else with the >> > same TCON, how one would identify such TCONs in the wild other than >> > EDID, if it relies on an external PWM or something, etc. And it might >> > make sense for now to make this a (shudder) driver-specific EDID quirk >> > rather than match by DPCD, at least until we know if the panel is ever >> > seen attached to other source devices and if the OUI convention is >> > self-contained. >> >> Thanks for clarifying. Pretty much agreed, unfortunately also on the >> "would be nice to know more" part... >> >> If this were to be an EDID quirk after all, I wonder if it would be >> better to store the parsed quirks to, say, struct drm_display_info, and >> have a drm_connector_has_quirk() function similar to drm_dp_has_quirk(). >> >> This would also allow us to not return quirks from >> drm_add_display_info(), which would arguably clean up the interface. > > Did anyone check if this is specified in the vbios? There appears to be a > field defined for this right... > > enum intel_backlight_type { > INTEL_BACKLIGHT_PMIC, > INTEL_BACKLIGHT_LPSS, > INTEL_BACKLIGHT_DISPLAY_DDI, > INTEL_BACKLIGHT_DSI_DCS, > INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */ > INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE, > }; Would just need /sys/kernel/debug/dri/0/i915_vbt on the affected machine to check. BR, Jani. > >> >> BR, >> Jani. >> >> -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
I'm about to post some more review comments for the v2 version of this, but some comments down below... On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote: > On Mon, 07 Oct 2019, Adam Jackson wrote: > > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote: > > > > > 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. > > > > My issue isn't really with identifying the panel from EDID rather than > > DPCD, whichever identifier is most specific is probably the best thing > > to use. It's more that this quirk is identified in common code but only > > applied in one driver. If this panel were ever to be attached to some > > other source, they might well want to apply the same kind of fix. My > > (admittedly naïve) reading of the OUI handshake process is that when > > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink > > "I'm about to issue commands that conform to _this_ vendor's own > > conventions". If that convention communicates information that is > > entirely contained within AUXCH transactions (and doesn't, for example, > > require looking at some other strapping pin or external device) then in > > principle it doesn't matter if the source device "matches" that OUI; it > > would be legal for an AMD GPU to write the same sequence and expect the > > same reaction, should that panel be attached to an AMD GPU. > > > > So, it would be nice to know exactly what that protocol is meant to do, > > if it applies only to this specific panel or anything else with the > > same TCON, how one would identify such TCONs in the wild other than > > EDID, if it relies on an external PWM or something, etc. And it might > > make sense for now to make this a (shudder) driver-specific EDID quirk > > rather than match by DPCD, at least until we know if the panel is ever > > seen attached to other source devices and if the OUI convention is > > self-contained. > > Thanks for clarifying. Pretty much agreed, unfortunately also on the > "would be nice to know more" part... > > If this were to be an EDID quirk after all, I wonder if it would be > better to store the parsed quirks to, say, struct drm_display_info, and > have a drm_connector_has_quirk() function similar to drm_dp_has_quirk(). > > This would also allow us to not return quirks from > drm_add_display_info(), which would arguably clean up the interface. Did anyone check if this is specified in the vbios? There appears to be a field defined for this right... enum intel_backlight_type { INTEL_BACKLIGHT_PMIC, INTEL_BACKLIGHT_LPSS, INTEL_BACKLIGHT_DISPLAY_DDI, INTEL_BACKLIGHT_DSI_DCS, INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */ INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE, }; > > BR, > Jani. > > -- Cheers, Lyude Paul ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Tue, 08 Oct 2019, Jani Nikula wrote: >On Mon, 07 Oct 2019, Adam Jackson wrote: >> On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote: >> >>> 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. >> >> My issue isn't really with identifying the panel from EDID rather than >> DPCD, whichever identifier is most specific is probably the best thing >> to use. It's more that this quirk is identified in common code but only >> applied in one driver. If this panel were ever to be attached to some >> other source, they might well want to apply the same kind of fix. My >> (admittedly naïve) reading of the OUI handshake process is that when >> the source device writes an OUI to DP_SOURCE_OUI it is telling the sink >> "I'm about to issue commands that conform to _this_ vendor's own >> conventions". If that convention communicates information that is >> entirely contained within AUXCH transactions (and doesn't, for example, >> require looking at some other strapping pin or external device) then in >> principle it doesn't matter if the source device "matches" that OUI; it >> would be legal for an AMD GPU to write the same sequence and expect the >> same reaction, should that panel be attached to an AMD GPU. >> >> So, it would be nice to know exactly what that protocol is meant to do, >> if it applies only to this specific panel or anything else with the >> same TCON, how one would identify such TCONs in the wild other than >> EDID, if it relies on an external PWM or something, etc. And it might >> make sense for now to make this a (shudder) driver-specific EDID quirk >> rather than match by DPCD, at least until we know if the panel is ever >> seen attached to other source devices and if the OUI convention is >> self-contained. > >Thanks for clarifying. Pretty much agreed, unfortunately also on the >"would be nice to know more" part... > >If this were to be an EDID quirk after all, I wonder if it would be >better to store the parsed quirks to, say, struct drm_display_info, and >have a drm_connector_has_quirk() function similar to drm_dp_has_quirk(). > >This would also allow us to not return quirks from >drm_add_display_info(), which would arguably clean up the interface. > >BR, >Jani. Thanks for advice! I've already update patch V2. Driver will check sink OUI and confirm TCON's capability to decide to enable this method or not. It depends on TCON's feature description and does not export EDID quirk. Best regards, Shawn ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Mon, 07 Oct 2019, Adam Jackson wrote: > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote: > >> 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. > > My issue isn't really with identifying the panel from EDID rather than > DPCD, whichever identifier is most specific is probably the best thing > to use. It's more that this quirk is identified in common code but only > applied in one driver. If this panel were ever to be attached to some > other source, they might well want to apply the same kind of fix. My > (admittedly naïve) reading of the OUI handshake process is that when > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink > "I'm about to issue commands that conform to _this_ vendor's own > conventions". If that convention communicates information that is > entirely contained within AUXCH transactions (and doesn't, for example, > require looking at some other strapping pin or external device) then in > principle it doesn't matter if the source device "matches" that OUI; it > would be legal for an AMD GPU to write the same sequence and expect the > same reaction, should that panel be attached to an AMD GPU. > > So, it would be nice to know exactly what that protocol is meant to do, > if it applies only to this specific panel or anything else with the > same TCON, how one would identify such TCONs in the wild other than > EDID, if it relies on an external PWM or something, etc. And it might > make sense for now to make this a (shudder) driver-specific EDID quirk > rather than match by DPCD, at least until we know if the panel is ever > seen attached to other source devices and if the OUI convention is > self-contained. Thanks for clarifying. Pretty much agreed, unfortunately also on the "would be nice to know more" part... If this were to be an EDID quirk after all, I wonder if it would be better to store the parsed quirks to, say, struct drm_display_info, and have a drm_connector_has_quirk() function similar to drm_dp_has_quirk(). This would also allow us to not return quirks from drm_add_display_info(), which would arguably clean up the interface. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote: > 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. My issue isn't really with identifying the panel from EDID rather than DPCD, whichever identifier is most specific is probably the best thing to use. It's more that this quirk is identified in common code but only applied in one driver. If this panel were ever to be attached to some other source, they might well want to apply the same kind of fix. My (admittedly naïve) reading of the OUI handshake process is that when the source device writes an OUI to DP_SOURCE_OUI it is telling the sink "I'm about to issue commands that conform to _this_ vendor's own conventions". If that convention communicates information that is entirely contained within AUXCH transactions (and doesn't, for example, require looking at some other strapping pin or external device) then in principle it doesn't matter if the source device "matches" that OUI; it would be legal for an AMD GPU to write the same sequence and expect the same reaction, should that panel be attached to an AMD GPU. So, it would be nice to know exactly what that protocol is meant to do, if it applies only to this specific panel or anything else with the same TCON, how one would identify such TCONs in the wild other than EDID, if it relies on an external PWM or something, etc. And it might make sense for now to make this a (shudder) driver-specific EDID quirk rather than match by DPCD, at least until we know if the panel is ever seen attached to other source devices and if the OUI convention is self-contained. - ajax ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Mon, 07 Oct 2019, "Jani Nikula" mailto:jani.nik...@intel.com>> wrote: >On Mon, 07 Oct 2019, "Lee, Shawn C" >mailto:shawn.c@intel.com>> wrote: >> On Fri, 04 Oct 2019, Jani Nikula >> mailto:jani.nik...@intel.com>> wrote: >>>On Fri, 04 Oct 2019, Adam Jackson 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. >> 0400 ba 41 59 00 00 00 00 00 00 00 00 00 00 00 00 00 >> |.AY.| >> 0410 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_LUMINANCE0x346 > +#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
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Mon, 07 Oct 2019, "Jani Nikula" wrote: >On Mon, 07 Oct 2019, "Lee, Shawn C" wrote: >> On Fri, 04 Oct 2019, Jani Nikula wrote: >>>On Fri, 04 Oct 2019, Adam Jackson 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. >> 0400 ba 41 59 00 00 00 00 00 00 00 00 00 00 00 00 00 >> |.AY.| >> 0410 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_LUMINANCE0x346 > +#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 >>>
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Mon, 07 Oct 2019, "Lee, Shawn C" wrote: > On Fri, 04 Oct 2019, Jani Nikula wrote: >>On Fri, 04 Oct 2019, Adam Jackson 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. > 0400 ba 41 59 00 00 00 00 00 00 00 00 00 00 00 00 00 |.AY.| > 0410 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_LUMINANCE0x346 +#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. > >> >>-- >>Jani Nikula, Intel Open Source Graphics Center >> -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Fri, 04 Oct 2019, Jani Nikula wrote: >On Fri, 04 Oct 2019, Adam Jackson 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. 0400 ba 41 59 00 00 00 00 00 00 00 00 00 00 00 00 00 |.AY.| 0410 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_PARAMS0x344 >>> +#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 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
On Fri, 04 Oct 2019, Adam Jackson 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. > >> @@ -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. > >> +#define DPCD_EDP_GETSET_CTRL_PARAMS 0x344 >> +#define DPCD_EDP_CONTENT_LUMINANCE 0x346 >> +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE 0x34a >> +#define DPCD_EDP_BRIGHTNESS_NITS0x354 >> +#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION0x358 >> + >> +#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? 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. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel
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. > + /* 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... > @@ -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. > +#define DPCD_EDP_GETSET_CTRL_PARAMS 0x344 > +#define DPCD_EDP_CONTENT_LUMINANCE 0x346 > +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE0x34a > +#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? - ajax ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx