Re: [v10 09/12] drm/i915:Enabled Modeset when HDR Infoframe changes

Ville Syrjälä
2019-05-16T13:13:12Z

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

--
Ville Syrjälä
Intel