| Message ID | 20220412052542.681419-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Check EDID before dpcd for possible HDR aux bl support | expand |
On Tue, 12 Apr 2022, Jouni Högander <jouni.hogander@intel.com> wrote: > We have now seen panel (XMG Core 15 e21 laptop) avertizing support > for Intel proprietary eDP backlight control via DPCD registers, but > actually working only with legacy pwm control. > > This patch adds panel EDID check for possible HDR static metadata and > does detection from DPCD registers only if this data block exists. > > Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight interface (only SDR for now)") > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5284 > Cc: Lyude Paul <lyude@redhat.com> > Cc: Mika Kahola <mika.kahola@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Tested-by: Filippo Falezza <filippo.falezza@outlook.it> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > index 97cf3cac0105..f69e185b58c1 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -108,6 +108,19 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > int ret; > u8 tcon_cap[4]; > > + /* > + * If we don't have HDR static metadata there is no way to > + * runtime detect used range for nits based control. For now > + * do not use Intel proprietary eDP backlight control if we > + * don't have this data in panel EDID. In case we find panel > + * which supports only nits based control, but doesn't provide > + * HDR static metadata we need to start maintaining table of > + * ranges for such panels. > + */ > + if (!(connector->base.hdr_sink_metadata.hdmi_type1.metadata_type & > + BIT(HDMI_STATIC_METADATA_TYPE1))) > + return false; Considering the complexities around this, I'd probably start gathering the info in variables, then debug log all of it, with the conclusion the driver makes. It's makes future debugging much easier. Other than that, I guess Acked-by: Jani Nikula <jani.nikula@intel.com> because I don't really know what's going on with these... BR, Jani. > + > intel_dp_wait_source_oui(intel_dp); > > ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));
On Tue, 2022-04-12 at 08:25 +0300, Jouni Högander wrote: > We have now seen panel (XMG Core 15 e21 laptop) avertizing support > for Intel proprietary eDP backlight control via DPCD registers, but > actually working only with legacy pwm control. > > This patch adds panel EDID check for possible HDR static metadata and > does detection from DPCD registers only if this data block exists. > > Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight interface > (only SDR for now)") > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5284 > Cc: Lyude Paul <lyude@redhat.com> > Cc: Mika Kahola <mika.kahola@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Tested-by: Filippo Falezza <filippo.falezza@outlook.it> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > index 97cf3cac0105..f69e185b58c1 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -108,6 +108,19 @@ intel_dp_aux_supports_hdr_backlight(struct > intel_connector *connector) > int ret; > u8 tcon_cap[4]; > > + /* > + * If we don't have HDR static metadata there is no way to > + * runtime detect used range for nits based control. For now > + * do not use Intel proprietary eDP backlight control if we > + * don't have this data in panel EDID. In case we find panel > + * which supports only nits based control, but doesn't provide > + * HDR static metadata we need to start maintaining table of > + * ranges for such panels. > + */ > + if (!(connector->base.hdr_sink_metadata.hdmi_type1.metadata_type & > + BIT(HDMI_STATIC_METADATA_TYPE1))) > + return false; The block used for this is actually for HDMI?? How bizarre… Anyway yeah - patch looks good to me, but I think we should print a debugging message of some sort when we determine that there's no HDR backlight because of the EDID - along with printing instructions for how the user can override it if we've made the wrong choice along with reporting a bug. Also - we should have the Cc: stable@vger.kernel.org tag from dim added here using `dim fixes $commit`. With that fixed: Reviewed-by: Lyude Paul <lyude@redhat.com> > + > intel_dp_wait_source_oui(intel_dp); > > ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, > sizeof(tcon_cap));
Hello Lyude, See my respose below. On Tue, 2022-04-12 at 13:50 -0400, Lyude Paul wrote: > On Tue, 2022-04-12 at 08:25 +0300, Jouni Högander wrote: > > We have now seen panel (XMG Core 15 e21 laptop) avertizing support > > for Intel proprietary eDP backlight control via DPCD registers, but > > actually working only with legacy pwm control. > > > > This patch adds panel EDID check for possible HDR static metadata > > and > > does detection from DPCD registers only if this data block exists. > > > > Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight > > interface > > (only SDR for now)") > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5284 > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: Mika Kahola <mika.kahola@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Tested-by: Filippo Falezza <filippo.falezza@outlook.it> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 13 > > +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > index 97cf3cac0105..f69e185b58c1 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > @@ -108,6 +108,19 @@ intel_dp_aux_supports_hdr_backlight(struct > > intel_connector *connector) > > int ret; > > u8 tcon_cap[4]; > > > > + /* > > + * If we don't have HDR static metadata there is no way to > > + * runtime detect used range for nits based control. For > > now > > + * do not use Intel proprietary eDP backlight control if we > > + * don't have this data in panel EDID. In case we find > > panel > > + * which supports only nits based control, but doesn't > > provide > > + * HDR static metadata we need to start maintaining table > > of > > + * ranges for such panels. > > + */ > > + if (!(connector- > > >base.hdr_sink_metadata.hdmi_type1.metadata_type & > > + BIT(HDMI_STATIC_METADATA_TYPE1))) > > + return false; > > The block used for this is actually for HDMI?? How bizarre… > > Anyway yeah - patch looks good to me, but I think we should print a > debugging > message of some sort when we determine that there's no HDR backlight > because > of the EDID - along with printing instructions for how the user can > override > it if we've made the wrong choice along with reporting a bug. Also - > we should > have the hmm, currently there is no override possibility in intel_dp_aux_supports_hdr_backlight. Do you think I should add one? I sent version 2. Didn't add your rb there as I wasn't sure if I understood your comment correctly. Please check new version. > Cc: stable@vger.kernel.org tag from dim added here using `dim fixes $commit`. With that fixed: Reviewed-by: Lyude Paul <lyude@redhat.com> + intel_dp_wait_source_oui(intel_dp); ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap)); BR, Jouni Högander
On Wed, 2022-04-13 at 08:31 +0000, Hogander, Jouni wrote: > Hello Lyude, > > See my respose below. > > On Tue, 2022-04-12 at 13:50 -0400, Lyude Paul wrote: > > On Tue, 2022-04-12 at 08:25 +0300, Jouni Högander wrote: > > > We have now seen panel (XMG Core 15 e21 laptop) avertizing support > > > for Intel proprietary eDP backlight control via DPCD registers, but > > > actually working only with legacy pwm control. > > > > > > This patch adds panel EDID check for possible HDR static metadata > > > and > > > does detection from DPCD registers only if this data block exists. > > > > > > Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight > > > interface > > > (only SDR for now)") > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5284 > > > Cc: Lyude Paul <lyude@redhat.com> > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Tested-by: Filippo Falezza <filippo.falezza@outlook.it> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 13 > > > +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > index 97cf3cac0105..f69e185b58c1 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > @@ -108,6 +108,19 @@ intel_dp_aux_supports_hdr_backlight(struct > > > intel_connector *connector) > > > int ret; > > > u8 tcon_cap[4]; > > > > > > + /* > > > + * If we don't have HDR static metadata there is no way to > > > + * runtime detect used range for nits based control. For > > > now > > > + * do not use Intel proprietary eDP backlight control if we > > > + * don't have this data in panel EDID. In case we find > > > panel > > > + * which supports only nits based control, but doesn't > > > provide > > > + * HDR static metadata we need to start maintaining table > > > of > > > + * ranges for such panels. > > > + */ > > > + if (!(connector- > > > > base.hdr_sink_metadata.hdmi_type1.metadata_type & > > > + BIT(HDMI_STATIC_METADATA_TYPE1))) > > > + return false; > > > > The block used for this is actually for HDMI?? How bizarre… > > > > Anyway yeah - patch looks good to me, but I think we should print a > > debugging > > message of some sort when we determine that there's no HDR backlight > > because > > of the EDID - along with printing instructions for how the user can > > override > > it if we've made the wrong choice along with reporting a bug. Also - > > we should > > have the > > hmm, currently there is no override possibility > in intel_dp_aux_supports_hdr_backlight. Do you think I should add one? Yes, probably - I think just making it so that i915.enable_dpcd_backlight=3 enables the HDR backlight regardless of the results of the EDID check would probably be a good idea. > > I sent version 2. Didn't add your rb there as I wasn't sure if I > understood your comment correctly. Please check new version. > > > Cc: stable@vger.kernel.org > > tag from dim added here using `dim fixes $commit`. > > With that fixed: > > Reviewed-by: Lyude Paul <lyude@redhat.com> > > + > intel_dp_wait_source_oui(intel_dp); > > ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, > tcon_cap, > sizeof(tcon_cap)); > > BR, > > Jouni Högander >
On Wed, 2022-04-13 at 17:08 -0400, Lyude Paul wrote: > On Wed, 2022-04-13 at 08:31 +0000, Hogander, Jouni wrote: > > Hello Lyude, > > > > See my respose below. > > > > On Tue, 2022-04-12 at 13:50 -0400, Lyude Paul wrote: > > > On Tue, 2022-04-12 at 08:25 +0300, Jouni Högander wrote: > > > > We have now seen panel (XMG Core 15 e21 laptop) avertizing > > > > support > > > > for Intel proprietary eDP backlight control via DPCD registers, > > > > but > > > > actually working only with legacy pwm control. > > > > > > > > This patch adds panel EDID check for possible HDR static > > > > metadata > > > > and > > > > does detection from DPCD registers only if this data block > > > > exists. > > > > > > > > Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight > > > > interface > > > > (only SDR for now)") > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5284 > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Tested-by: Filippo Falezza <filippo.falezza@outlook.it> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > > --- > > > > .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 13 > > > > +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git > > > > a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > index 97cf3cac0105..f69e185b58c1 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > > > > @@ -108,6 +108,19 @@ intel_dp_aux_supports_hdr_backlight(struct > > > > intel_connector *connector) > > > > int ret; > > > > u8 tcon_cap[4]; > > > > > > > > + /* > > > > + * If we don't have HDR static metadata there is no way > > > > to > > > > + * runtime detect used range for nits based control. > > > > For > > > > now > > > > + * do not use Intel proprietary eDP backlight control > > > > if we > > > > + * don't have this data in panel EDID. In case we find > > > > panel > > > > + * which supports only nits based control, but doesn't > > > > provide > > > > + * HDR static metadata we need to start maintaining > > > > table > > > > of > > > > + * ranges for such panels. > > > > + */ > > > > + if (!(connector- > > > > > base.hdr_sink_metadata.hdmi_type1.metadata_type & > > > > + BIT(HDMI_STATIC_METADATA_TYPE1))) > > > > + return false; > > > > > > The block used for this is actually for HDMI?? How bizarre… > > > > > > Anyway yeah - patch looks good to me, but I think we should print > > > a > > > debugging > > > message of some sort when we determine that there's no HDR > > > backlight > > > because > > > of the EDID - along with printing instructions for how the user > > > can > > > override > > > it if we've made the wrong choice along with reporting a bug. > > > Also - > > > we should > > > have the > > > > hmm, currently there is no override possibility > > in intel_dp_aux_supports_hdr_backlight. Do you think I should add > > one? > > Yes, probably - I think just making it so that > i915.enable_dpcd_backlight=3 > enables the HDR backlight regardless of the results of the EDID check > would > probably be a good idea. Ok, I think I understood you correctly originally. If you could please check version 2: https://patchwork.freedesktop.org/patch/481985/?series=102645&rev=1 > > > I sent version 2. Didn't add your rb there as I wasn't sure if I > > understood your comment correctly. Please check new version. > > > > > Cc: stable@vger.kernel.org > > > > tag from dim added here using `dim fixes $commit`. > > > > With that fixed: > > > > Reviewed-by: Lyude Paul <lyude@redhat.com> > > > > + > > intel_dp_wait_source_oui(intel_dp); > > > > ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, > > tcon_cap, > > sizeof(tcon_cap)); > > > > BR, > > > > Jouni Högander > >
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c index 97cf3cac0105..f69e185b58c1 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c @@ -108,6 +108,19 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) int ret; u8 tcon_cap[4]; + /* + * If we don't have HDR static metadata there is no way to + * runtime detect used range for nits based control. For now + * do not use Intel proprietary eDP backlight control if we + * don't have this data in panel EDID. In case we find panel + * which supports only nits based control, but doesn't provide + * HDR static metadata we need to start maintaining table of + * ranges for such panels. + */ + if (!(connector->base.hdr_sink_metadata.hdmi_type1.metadata_type & + BIT(HDMI_STATIC_METADATA_TYPE1))) + return false; + intel_dp_wait_source_oui(intel_dp); ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));
We have now seen panel (XMG Core 15 e21 laptop) avertizing support for Intel proprietary eDP backlight control via DPCD registers, but actually working only with legacy pwm control. This patch adds panel EDID check for possible HDR static metadata and does detection from DPCD registers only if this data block exists. Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight interface (only SDR for now)") Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5284 Cc: Lyude Paul <lyude@redhat.com> Cc: Mika Kahola <mika.kahola@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Tested-by: Filippo Falezza <filippo.falezza@outlook.it> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)