| Message ID | 1557855394-12214-10-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add HDR Metadata Parsing and handling in DRM layer | expand |
On Tue, May 14, 2019 at 11:06:31PM +0530, Uma Shankar wrote: > This patch enables modeset whenever HDR metadata > needs to be updated to sink. > > v2: Addressed Shashank's review comments. > > v3: Added Shashank's RB. > > v4: Addressed Ville's review comments. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 14 +++++++++++++- > drivers/gpu/drm/i915/intel_hdmi.c | 13 +++++++++++++ > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 58b8049..6b985e8 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -105,6 +105,16 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, > return -EINVAL; > } > > +static bool blob_equal(const struct drm_property_blob *a, > + const struct drm_property_blob *b) > +{ > + if (a && b) > + return a->length == b->length && > + !memcmp(a->data, b->data, a->length); > + > + return !a == !b; > +} > + > int intel_digital_connector_atomic_check(struct drm_connector *conn, > struct drm_connector_state *new_state) > { > @@ -132,7 +142,9 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > new_conn_state->base.colorspace != old_conn_state->base.colorspace || > new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio || > new_conn_state->base.content_type != old_conn_state->base.content_type || > - new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode) > + new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode || > + !blob_equal(new_conn_state->base.hdr_output_metadata, > + old_conn_state->base.hdr_output_metadata)) > crtc_state->mode_changed = true; > > return 0; > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index b80406b..e97bf6e 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -806,6 +806,11 @@ void intel_read_infoframe(struct intel_encoder *encoder, > return true; > } > > +static inline bool is_eotf_supported(u8 output_eotf, u8 sink_eotf) > +{ > + return sink_eotf & BIT(output_eotf); > +} > + > static bool > intel_hdmi_compute_drm_infoframe(struct intel_encoder *encoder, > struct intel_crtc_state *crtc_state, > @@ -814,6 +819,7 @@ void intel_read_infoframe(struct intel_encoder *encoder, > struct hdmi_drm_infoframe *frame = &crtc_state->infoframes.drm.drm; > struct hdr_output_metadata *hdr_metadata; > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct drm_connector *connector = conn_state->connector; > int ret; > > if (!(INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))) > @@ -828,6 +834,13 @@ void intel_read_infoframe(struct intel_encoder *encoder, > > hdr_metadata = conn_state->hdr_output_metadata->data; > > + /* Sink EOTF is Bit map while infoframe is absolute values */ > + if (!is_eotf_supported(hdr_metadata->hdmi_metadata_type1.eotf, > + connector->hdr_sink_metadata.hdmi_type1.eotf)) { > + DRM_ERROR("EOTF Not Supported\n"); > + return true; > + } I was going to say that this should probably be in the drm_set_hdr_metdata() or whatever it was called. But now I'm now wondering if we can even have this check here. What happens if someone does a display switcheroo while the machine is suspended? Depends on when we're going to reprobe the displays I suppose. Hmm. Maybe it's fine. We already have a similar issue after all wih the has_hdmi2_sink stuff. Either way the user triggerable DRM_ERROR()s are at least a nono. > + > crtc_state->infoframes.enable |= > intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM); > > -- > 1.9.1
>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville >Syrjälä >Sent: Thursday, May 16, 2019 12:57 AM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: dcastagna@chromium.org; jonas@kwiboo.se; intel-gfx@lists.freedesktop.org; >emil.l.velikov@gmail.com; dri-devel@lists.freedesktop.org; seanpaul@chromium.org >Subject: Re: [v10 09/12] drm/i915:Enabled Modeset when HDR Infoframe changes > >On Tue, May 14, 2019 at 11:06:31PM +0530, Uma Shankar wrote: >> This patch enables modeset whenever HDR metadata needs to be updated >> to sink. >> >> v2: Addressed Shashank's review comments. >> >> v3: Added Shashank's RB. >> >> v4: Addressed Ville's review comments. >> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_atomic.c | 14 +++++++++++++- >> drivers/gpu/drm/i915/intel_hdmi.c | 13 +++++++++++++ >> 2 files changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c >> b/drivers/gpu/drm/i915/intel_atomic.c >> index 58b8049..6b985e8 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -105,6 +105,16 @@ int intel_digital_connector_atomic_set_property(struct >drm_connector *connector, >> return -EINVAL; >> } >> >> +static bool blob_equal(const struct drm_property_blob *a, >> + const struct drm_property_blob *b) { >> + if (a && b) >> + return a->length == b->length && >> + !memcmp(a->data, b->data, a->length); >> + >> + return !a == !b; >> +} >> + >> int intel_digital_connector_atomic_check(struct drm_connector *conn, >> struct drm_connector_state *new_state) { >@@ -132,7 +142,9 @@ >> int intel_digital_connector_atomic_check(struct drm_connector *conn, >> new_conn_state->base.colorspace != old_conn_state->base.colorspace || >> new_conn_state->base.picture_aspect_ratio != old_conn_state- >>base.picture_aspect_ratio || >> new_conn_state->base.content_type != old_conn_state- >>base.content_type || >> - new_conn_state->base.scaling_mode != old_conn_state- >>base.scaling_mode) >> + new_conn_state->base.scaling_mode != old_conn_state- >>base.scaling_mode || >> + !blob_equal(new_conn_state->base.hdr_output_metadata, >> + old_conn_state->base.hdr_output_metadata)) >> crtc_state->mode_changed = true; >> >> return 0; >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c >> b/drivers/gpu/drm/i915/intel_hdmi.c >> index b80406b..e97bf6e 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -806,6 +806,11 @@ void intel_read_infoframe(struct intel_encoder *encoder, >> return true; >> } >> >> +static inline bool is_eotf_supported(u8 output_eotf, u8 sink_eotf) { >> + return sink_eotf & BIT(output_eotf); } >> + >> static bool >> intel_hdmi_compute_drm_infoframe(struct intel_encoder *encoder, >> struct intel_crtc_state *crtc_state, @@ -814,6 >+819,7 @@ void >> intel_read_infoframe(struct intel_encoder *encoder, >> struct hdmi_drm_infoframe *frame = &crtc_state->infoframes.drm.drm; >> struct hdr_output_metadata *hdr_metadata; >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> + struct drm_connector *connector = conn_state->connector; >> int ret; >> >> if (!(INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))) @@ >> -828,6 +834,13 @@ void intel_read_infoframe(struct intel_encoder >> *encoder, >> >> hdr_metadata = conn_state->hdr_output_metadata->data; >> >> + /* Sink EOTF is Bit map while infoframe is absolute values */ >> + if (!is_eotf_supported(hdr_metadata->hdmi_metadata_type1.eotf, >> + connector->hdr_sink_metadata.hdmi_type1.eotf)) { >> + DRM_ERROR("EOTF Not Supported\n"); >> + return true; >> + } > >I was going to say that this should probably be in the >drm_set_hdr_metdata() or whatever it was called. > >But now I'm now wondering if we can even have this check here. What happens if >someone does a display switcheroo while the machine is suspended? Depends on >when we're going to reprobe the displays I suppose. Hmm. Maybe it's fine. We >already have a similar issue after all wih the has_hdmi2_sink stuff. > >Either way the user triggerable DRM_ERROR()s are at least a nono. Ok, Will keep the check and move it inside the drm_set_hdr_metdata(). Also downgrade the print as INFO instead of ERROR. Hope this is fine. >> + >> crtc_state->infoframes.enable |= >> intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM); >> >> -- >> 1.9.1 > >-- >Ville Syrjälä >Intel >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, May 16, 2019 at 10:54:14AM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Ville > >Syrjälä > >Sent: Thursday, May 16, 2019 12:57 AM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: dcastagna@chromium.org; jonas@kwiboo.se; intel-gfx@lists.freedesktop.org; > >emil.l.velikov@gmail.com; dri-devel@lists.freedesktop.org; seanpaul@chromium.org > >Subject: Re: [v10 09/12] drm/i915:Enabled Modeset when HDR Infoframe changes > > > >On Tue, May 14, 2019 at 11:06:31PM +0530, Uma Shankar wrote: > >> This patch enables modeset whenever HDR metadata needs to be updated > >> to sink. > >> > >> v2: Addressed Shashank's review comments. > >> > >> v3: Added Shashank's RB. > >> > >> v4: Addressed Ville's review comments. > >> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> > >> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_atomic.c | 14 +++++++++++++- > >> drivers/gpu/drm/i915/intel_hdmi.c | 13 +++++++++++++ > >> 2 files changed, 26 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c > >> b/drivers/gpu/drm/i915/intel_atomic.c > >> index 58b8049..6b985e8 100644 > >> --- a/drivers/gpu/drm/i915/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/intel_atomic.c > >> @@ -105,6 +105,16 @@ int intel_digital_connector_atomic_set_property(struct > >drm_connector *connector, > >> return -EINVAL; > >> } > >> > >> +static bool blob_equal(const struct drm_property_blob *a, > >> + const struct drm_property_blob *b) { > >> + if (a && b) > >> + return a->length == b->length && > >> + !memcmp(a->data, b->data, a->length); > >> + > >> + return !a == !b; > >> +} > >> + > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > >> struct drm_connector_state *new_state) { > >@@ -132,7 +142,9 @@ > >> int intel_digital_connector_atomic_check(struct drm_connector *conn, > >> new_conn_state->base.colorspace != old_conn_state->base.colorspace || > >> new_conn_state->base.picture_aspect_ratio != old_conn_state- > >>base.picture_aspect_ratio || > >> new_conn_state->base.content_type != old_conn_state- > >>base.content_type || > >> - new_conn_state->base.scaling_mode != old_conn_state- > >>base.scaling_mode) > >> + new_conn_state->base.scaling_mode != old_conn_state- > >>base.scaling_mode || > >> + !blob_equal(new_conn_state->base.hdr_output_metadata, > >> + old_conn_state->base.hdr_output_metadata)) > >> crtc_state->mode_changed = true; > >> > >> return 0; > >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > >> b/drivers/gpu/drm/i915/intel_hdmi.c > >> index b80406b..e97bf6e 100644 > >> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >> @@ -806,6 +806,11 @@ void intel_read_infoframe(struct intel_encoder *encoder, > >> return true; > >> } > >> > >> +static inline bool is_eotf_supported(u8 output_eotf, u8 sink_eotf) { > >> + return sink_eotf & BIT(output_eotf); } > >> + > >> static bool > >> intel_hdmi_compute_drm_infoframe(struct intel_encoder *encoder, > >> struct intel_crtc_state *crtc_state, @@ -814,6 > >+819,7 @@ void > >> intel_read_infoframe(struct intel_encoder *encoder, > >> struct hdmi_drm_infoframe *frame = &crtc_state->infoframes.drm.drm; > >> struct hdr_output_metadata *hdr_metadata; > >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> + struct drm_connector *connector = conn_state->connector; > >> int ret; > >> > >> if (!(INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))) @@ > >> -828,6 +834,13 @@ void intel_read_infoframe(struct intel_encoder > >> *encoder, > >> > >> hdr_metadata = conn_state->hdr_output_metadata->data; > >> > >> + /* Sink EOTF is Bit map while infoframe is absolute values */ > >> + if (!is_eotf_supported(hdr_metadata->hdmi_metadata_type1.eotf, > >> + connector->hdr_sink_metadata.hdmi_type1.eotf)) { > >> + DRM_ERROR("EOTF Not Supported\n"); > >> + return true; > >> + } > > > >I was going to say that this should probably be in the > >drm_set_hdr_metdata() or whatever it was called. > > > >But now I'm now wondering if we can even have this check here. What happens if > >someone does a display switcheroo while the machine is suspended? Depends on > >when we're going to reprobe the displays I suppose. Hmm. Maybe it's fine. We > >already have a similar issue after all wih the has_hdmi2_sink stuff. > > > >Either way the user triggerable DRM_ERROR()s are at least a nono. > > Ok, Will keep the check and move it inside the drm_set_hdr_metdata(). Also downgrade > the print as INFO instead of ERROR. Hope this is fine. DEBUG_KMS like everything else. > > >> + > >> crtc_state->infoframes.enable |= > >> intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM); > >> > >> -- > >> 1.9.1 > > > >-- > >Ville Syrjälä > >Intel > >_______________________________________________ > >dri-devel mailing list > >dri-devel@lists.freedesktop.org > >https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 58b8049..6b985e8 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -105,6 +105,16 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, return -EINVAL; } +static bool blob_equal(const struct drm_property_blob *a, + const struct drm_property_blob *b) +{ + if (a && b) + return a->length == b->length && + !memcmp(a->data, b->data, a->length); + + return !a == !b; +} + int intel_digital_connector_atomic_check(struct drm_connector *conn, struct drm_connector_state *new_state) { @@ -132,7 +142,9 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, new_conn_state->base.colorspace != old_conn_state->base.colorspace || new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio || new_conn_state->base.content_type != old_conn_state->base.content_type || - new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode) + new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode || + !blob_equal(new_conn_state->base.hdr_output_metadata, + old_conn_state->base.hdr_output_metadata)) crtc_state->mode_changed = true; return 0; diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index b80406b..e97bf6e 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -806,6 +806,11 @@ void intel_read_infoframe(struct intel_encoder *encoder, return true; } +static inline bool is_eotf_supported(u8 output_eotf, u8 sink_eotf) +{ + return sink_eotf & BIT(output_eotf); +} + static bool intel_hdmi_compute_drm_infoframe(struct intel_encoder *encoder, struct intel_crtc_state *crtc_state, @@ -814,6 +819,7 @@ void intel_read_infoframe(struct intel_encoder *encoder, struct hdmi_drm_infoframe *frame = &crtc_state->infoframes.drm.drm; struct hdr_output_metadata *hdr_metadata; struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct drm_connector *connector = conn_state->connector; int ret; if (!(INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))) @@ -828,6 +834,13 @@ void intel_read_infoframe(struct intel_encoder *encoder, hdr_metadata = conn_state->hdr_output_metadata->data; + /* Sink EOTF is Bit map while infoframe is absolute values */ + if (!is_eotf_supported(hdr_metadata->hdmi_metadata_type1.eotf, + connector->hdr_sink_metadata.hdmi_type1.eotf)) { + DRM_ERROR("EOTF Not Supported\n"); + return true; + } + crtc_state->infoframes.enable |= intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM);