>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Tuesday, May 14, 2019 1:06 AM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; >dcastagna@xxxxxxxxxxxx; jonas@xxxxxxxxx; emil.l.velikov@xxxxxxxxx; >seanpaul@xxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >Subject: Re: [v9 06/13] drm/i915: Write HDR infoframe and send to panel > >On Thu, May 09, 2019 at 12:08:46AM +0530, Uma Shankar wrote: >> Enable writing of HDR metadata infoframe to panel. >> The data will be provid by usersapace compositors, based on blending >> policies and passsed to driver through a blob property. >> >> v2: Rebase >> >> v3: Fixed a warning message >> >> v4: Addressed Shashank's review comments >> >> v5: Rebase. Added infoframe calculation in compute config. >> >> v6: Addressed Shashank's review comment. Added HDR metadata support >> from GEN10 onwards as per Shashank's recommendation. >> >> v7: Addressed Shashank's review comments >> >> v8: Added Shashank's RB. >> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> drivers/gpu/drm/i915/intel_hdmi.c | 48 >> +++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 49 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h >> b/drivers/gpu/drm/i915/intel_drv.h >> index 247893e..bc32b2c 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -910,6 +910,7 @@ struct intel_crtc_state { >> union hdmi_infoframe avi; >> union hdmi_infoframe spd; >> union hdmi_infoframe hdmi; >> + union hdmi_infoframe drm; >> } infoframes; >> >> /* HDMI scrambling status */ >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c >> b/drivers/gpu/drm/i915/intel_hdmi.c >> index 92597d8..980900b 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -573,6 +573,7 @@ static u32 hsw_infoframes_enabled(struct intel_encoder >*encoder, >> HDMI_INFOFRAME_TYPE_AVI, >> HDMI_INFOFRAME_TYPE_SPD, >> HDMI_INFOFRAME_TYPE_VENDOR, >> + HDMI_INFOFRAME_TYPE_DRM, >> }; >> >> u32 intel_hdmi_infoframe_enable(unsigned int type) @@ -795,6 +796,30 >> @@ void intel_read_infoframe(struct intel_encoder *encoder, >> return true; >> } >> >> +static bool >> +intel_hdmi_compute_drm_infoframe(struct intel_encoder *encoder, >> + struct intel_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state) { >> + struct hdmi_drm_infoframe *frame = &crtc_state->infoframes.drm.drm; >> + struct hdr_output_metadata *hdr_metadata; >> + int ret; > >Missing has_infoframes check. Ok, will add this. >> + >> + hdr_metadata = (struct hdr_output_metadata *) > >Pointless cast? Yeah, will remove this. > >> + conn_state->hdr_output_metadata->data; > >NULL pointer deref? Hmm, how does this pass CI? Actually this check is added in a later patch in series (patch 9), will move the check here to avoid any bisect issues. > >> + >> + ret = drm_hdmi_infoframe_set_hdr_metadata(frame, hdr_metadata); >> + if (ret < 0) { >> + DRM_ERROR("couldn't set HDR metadata in infoframe\n"); >> + return false; >> + } >> + >> + crtc_state->infoframes.enable |= >> + intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_DRM); > >Everyone else sets this before populating the infoframe. Will move this up. >Missing the whatever_infoframe_check() call here. Sure, will add the check. >> + >> + return true; >> +} >> + >> static void g4x_set_infoframes(struct intel_encoder *encoder, >> bool enable, >> const struct intel_crtc_state *crtc_state, @@ -1180,6 >> +1205,16 @@ static void hsw_set_infoframes(struct intel_encoder *encoder, >> intel_write_infoframe(encoder, crtc_state, >> HDMI_INFOFRAME_TYPE_VENDOR, >> &crtc_state->infoframes.hdmi); >> + >> + /* >> + * Support HDR Metadata from Gen10 onwards >> + * ToDo: Gen9 also can support HDR with LSPCON. >> + * Support for the same to be enabled later. >> + */ >> + if (INTEL_GEN(dev_priv) >= 10) > >Missing glk. Actually this check can be removed entirely since >intel_write_infoframe() already checks infoframes.enable. Yeah right, will drop this check altogether. >> + intel_write_infoframe(encoder, crtc_state, >> + HDMI_INFOFRAME_TYPE_DRM, >> + &crtc_state->infoframes.drm); >> } >> >> void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool >> enable) @@ -2386,6 +2421,19 @@ int intel_hdmi_compute_config(struct >intel_encoder *encoder, >> return -EINVAL; >> } >> >> + /* >> + * Support HDR Metadata from Gen10 onwards > >This is not providing extra information beyond what the code is saying. >> + * ToDo: Gen9 also can support HDR with LSPCON. >> + * Support for the same to be enabled later. > >This is native HDMI code. LSPCON fixme doesn't make sense here. Ok, will drop the comments here. >> + */ >> + if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { > >For GCP the platform check is in the function itself. Could do the same here for >consistency. Sure, will do the same. Regards, Uma Shankar >> + if (!intel_hdmi_compute_drm_infoframe(encoder, pipe_config, >> + conn_state)) { >> + DRM_DEBUG_KMS("bad DRM infoframe\n"); >> + return -EINVAL; >> + } >> + } >> + >> return 0; >> } >> >> -- >> 1.9.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >-- >Ville Syrjälä >Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
