>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Ville >Syrjälä >Sent: Thursday, May 16, 2019 12:57 AM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: dcastagna@xxxxxxxxxxxx; jonas@xxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >emil.l.velikov@xxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; seanpaul@xxxxxxxxxxxx >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@xxxxxxxxxxxxxxx> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx >https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
