On Thu, May 16, 2019 at 10:54:14AM +0000, Shankar, Uma wrote: > > > >-----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. 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@xxxxxxxxxxxxxxxxxxxxx > >https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
