| Message ID | 20200914210047.11972-3-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable HDR on MCA LSPCON based Gen9 devices | expand |
On Tue, Sep 15, 2020 at 02:30:38AM +0530, Uma Shankar wrote: > Gen9 hardware supports HDMI2.0 through LSPCON chips. > Extending HDR support for MCA LSPCON based GEN9 devices. > > SOC will drive LSPCON as DP and send HDR metadata as standard > DP SDP packets. LSPCON will be set to operate in PCON mode, > will receive the metadata and create Dynamic Range and > Mastering Infoframe (DRM packets) and send it to HDR capable > HDMI sink devices. > > v2: Re-used hsw infoframe write implementation for HDR metadata > for LSPCON as per Ville's suggestion. > > v3: Addressed Jani Nikula's review comments. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/i915/display/intel_hdmi.c | 10 ++++++ > drivers/gpu/drm/i915/display/intel_lspcon.c | 37 +++++++++++++++------ > drivers/gpu/drm/i915/display/intel_lspcon.h | 5 ++- > 3 files changed, 40 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 0978b0d8f4c6..1e40ed473fb9 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -590,6 +590,16 @@ static u32 hsw_infoframes_enabled(struct intel_encoder *encoder, > return val & mask; > } > > +void lspcon_drm_write_infoframe(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + unsigned int type, > + const void *frame, ssize_t len) > +{ > + drm_dbg_kms(encoder->base.dev, "Update HDR metadata for lspcon\n"); > + /* It uses the legacy hsw implementation for the same */ > + hsw_write_infoframe(encoder, crtc_state, type, frame, len); > +} This wrapper seems quite pointless. > + > static const u8 infoframe_type_to_idx[] = { > HDMI_PACKET_TYPE_GENERAL_CONTROL, > HDMI_PACKET_TYPE_GAMUT_METADATA, > diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c b/drivers/gpu/drm/i915/display/intel_lspcon.c > index 8e8c7a02ab51..5e2d7ca1d20f 100644 > --- a/drivers/gpu/drm/i915/display/intel_lspcon.c > +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c > @@ -461,27 +461,42 @@ void lspcon_write_infoframe(struct intel_encoder *encoder, > unsigned int type, > const void *frame, ssize_t len) > { > - bool ret; > + bool ret = true; > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder); > > - /* LSPCON only needs AVI IF */ > - if (type != HDMI_INFOFRAME_TYPE_AVI) > + /* > + * Supporting HDR on MCA LSPCON > + * Todo: Add support for Parade later > + */ > + if (type == HDMI_PACKET_TYPE_GAMUT_METADATA && > + lspcon->vendor != LSPCON_VENDOR_MCA) > return; We shouldn't have the infoframe flagged as enabled if we don't support it. So this check seems pointless, or there's a bug somewhere else. > > - if (lspcon->vendor == LSPCON_VENDOR_MCA) > - ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux, > - frame, len); > - else > - ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux, > - frame, len); > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > + if (lspcon->vendor == LSPCON_VENDOR_MCA) > + ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux, > + frame, len); > + else > + ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux, > + frame, len); > + break; > + case HDMI_PACKET_TYPE_GAMUT_METADATA: > + lspcon_drm_write_infoframe(encoder, crtc_state, > + HDMI_PACKET_TYPE_GAMUT_METADATA, > + frame, VIDEO_DIP_DATA_SIZE); Why are we hardocoding the parameters here? Just pass them through? > + break; > + default: > + return; > + } > > if (!ret) { > - DRM_ERROR("Failed to write AVI infoframes\n"); > + DRM_ERROR("Failed to write infoframes\n"); > return; > } > > - DRM_DEBUG_DRIVER("AVI infoframes updated successfully\n"); > + DRM_DEBUG_DRIVER("Infoframes updated successfully\n"); That pointless debug should probably be just nuked. > } > > void lspcon_read_infoframe(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.h b/drivers/gpu/drm/i915/display/intel_lspcon.h > index 1cffe8a42a08..3fac05535731 100644 > --- a/drivers/gpu/drm/i915/display/intel_lspcon.h > +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h > @@ -34,5 +34,8 @@ u32 lspcon_infoframes_enabled(struct intel_encoder *encoder, > const struct intel_crtc_state *pipe_config); > void lspcon_ycbcr420_config(struct drm_connector *connector, > struct intel_crtc_state *crtc_state); > - > +void lspcon_drm_write_infoframe(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + unsigned int type, > + const void *frame, ssize_t len); I think we ususally leave a blank line here. > #endif /* __INTEL_LSPCON_H__ */ > -- > 2.26.2
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Tuesday, September 29, 2020 9:42 PM > To: Shankar, Uma <uma.shankar@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [v6 02/11] drm/i915/display: Enable HDR on gen9 devices with MCA > Lspcon > > On Tue, Sep 15, 2020 at 02:30:38AM +0530, Uma Shankar wrote: > > Gen9 hardware supports HDMI2.0 through LSPCON chips. > > Extending HDR support for MCA LSPCON based GEN9 devices. > > > > SOC will drive LSPCON as DP and send HDR metadata as standard DP SDP > > packets. LSPCON will be set to operate in PCON mode, will receive the > > metadata and create Dynamic Range and Mastering Infoframe (DRM > > packets) and send it to HDR capable HDMI sink devices. > > > > v2: Re-used hsw infoframe write implementation for HDR metadata for > > LSPCON as per Ville's suggestion. > > > > v3: Addressed Jani Nikula's review comments. > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_hdmi.c | 10 ++++++ > > drivers/gpu/drm/i915/display/intel_lspcon.c | 37 > > +++++++++++++++------ drivers/gpu/drm/i915/display/intel_lspcon.h | > > 5 ++- > > 3 files changed, 40 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > > b/drivers/gpu/drm/i915/display/intel_hdmi.c > > index 0978b0d8f4c6..1e40ed473fb9 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > > @@ -590,6 +590,16 @@ static u32 hsw_infoframes_enabled(struct > intel_encoder *encoder, > > return val & mask; > > } > > > > +void lspcon_drm_write_infoframe(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + unsigned int type, > > + const void *frame, ssize_t len) > > +{ > > + drm_dbg_kms(encoder->base.dev, "Update HDR metadata for lspcon\n"); > > + /* It uses the legacy hsw implementation for the same */ > > + hsw_write_infoframe(encoder, crtc_state, type, frame, len); } > > This wrapper seems quite pointless. Hmm yeah, will drop this. > > + > > static const u8 infoframe_type_to_idx[] = { > > HDMI_PACKET_TYPE_GENERAL_CONTROL, > > HDMI_PACKET_TYPE_GAMUT_METADATA, > > diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c > > b/drivers/gpu/drm/i915/display/intel_lspcon.c > > index 8e8c7a02ab51..5e2d7ca1d20f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_lspcon.c > > +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c > > @@ -461,27 +461,42 @@ void lspcon_write_infoframe(struct intel_encoder > *encoder, > > unsigned int type, > > const void *frame, ssize_t len) { > > - bool ret; > > + bool ret = true; > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder); > > > > - /* LSPCON only needs AVI IF */ > > - if (type != HDMI_INFOFRAME_TYPE_AVI) > > + /* > > + * Supporting HDR on MCA LSPCON > > + * Todo: Add support for Parade later > > + */ > > + if (type == HDMI_PACKET_TYPE_GAMUT_METADATA && > > + lspcon->vendor != LSPCON_VENDOR_MCA) > > return; > > We shouldn't have the infoframe flagged as enabled if we don't support it. So > this check seems pointless, or there's a bug somewhere else. Sure, will drop this check. It's not required. > > > > - if (lspcon->vendor == LSPCON_VENDOR_MCA) > > - ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux, > > - frame, len); > > - else > > - ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux, > > - frame, len); > > + switch (type) { > > + case HDMI_INFOFRAME_TYPE_AVI: > > + if (lspcon->vendor == LSPCON_VENDOR_MCA) > > + ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux, > > + frame, len); > > + else > > + ret = _lspcon_write_avi_infoframe_parade(&intel_dp- > >aux, > > + frame, len); > > + break; > > + case HDMI_PACKET_TYPE_GAMUT_METADATA: > > + lspcon_drm_write_infoframe(encoder, crtc_state, > > + > HDMI_PACKET_TYPE_GAMUT_METADATA, > > + frame, VIDEO_DIP_DATA_SIZE); > > Why are we hardocoding the parameters here? Just pass them through? Ok, will rectify this. > > + break; > > + default: > > + return; > > + } > > > > if (!ret) { > > - DRM_ERROR("Failed to write AVI infoframes\n"); > > + DRM_ERROR("Failed to write infoframes\n"); > > return; > > } > > > > - DRM_DEBUG_DRIVER("AVI infoframes updated successfully\n"); > > + DRM_DEBUG_DRIVER("Infoframes updated successfully\n"); > > That pointless debug should probably be just nuked. Ok, will drop it. > > } > > > > void lspcon_read_infoframe(struct intel_encoder *encoder, diff --git > > a/drivers/gpu/drm/i915/display/intel_lspcon.h > > b/drivers/gpu/drm/i915/display/intel_lspcon.h > > index 1cffe8a42a08..3fac05535731 100644 > > --- a/drivers/gpu/drm/i915/display/intel_lspcon.h > > +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h > > @@ -34,5 +34,8 @@ u32 lspcon_infoframes_enabled(struct intel_encoder > *encoder, > > const struct intel_crtc_state *pipe_config); void > > lspcon_ycbcr420_config(struct drm_connector *connector, > > struct intel_crtc_state *crtc_state); > > - > > +void lspcon_drm_write_infoframe(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + unsigned int type, > > + const void *frame, ssize_t len); > > I think we ususally leave a blank line here. Will do it. > > #endif /* __INTEL_LSPCON_H__ */ > > -- > > 2.26.2 > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 0978b0d8f4c6..1e40ed473fb9 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -590,6 +590,16 @@ static u32 hsw_infoframes_enabled(struct intel_encoder *encoder, return val & mask; } +void lspcon_drm_write_infoframe(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, + unsigned int type, + const void *frame, ssize_t len) +{ + drm_dbg_kms(encoder->base.dev, "Update HDR metadata for lspcon\n"); + /* It uses the legacy hsw implementation for the same */ + hsw_write_infoframe(encoder, crtc_state, type, frame, len); +} + static const u8 infoframe_type_to_idx[] = { HDMI_PACKET_TYPE_GENERAL_CONTROL, HDMI_PACKET_TYPE_GAMUT_METADATA, diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c b/drivers/gpu/drm/i915/display/intel_lspcon.c index 8e8c7a02ab51..5e2d7ca1d20f 100644 --- a/drivers/gpu/drm/i915/display/intel_lspcon.c +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c @@ -461,27 +461,42 @@ void lspcon_write_infoframe(struct intel_encoder *encoder, unsigned int type, const void *frame, ssize_t len) { - bool ret; + bool ret = true; struct intel_dp *intel_dp = enc_to_intel_dp(encoder); struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder); - /* LSPCON only needs AVI IF */ - if (type != HDMI_INFOFRAME_TYPE_AVI) + /* + * Supporting HDR on MCA LSPCON + * Todo: Add support for Parade later + */ + if (type == HDMI_PACKET_TYPE_GAMUT_METADATA && + lspcon->vendor != LSPCON_VENDOR_MCA) return; - if (lspcon->vendor == LSPCON_VENDOR_MCA) - ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux, - frame, len); - else - ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux, - frame, len); + switch (type) { + case HDMI_INFOFRAME_TYPE_AVI: + if (lspcon->vendor == LSPCON_VENDOR_MCA) + ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux, + frame, len); + else + ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux, + frame, len); + break; + case HDMI_PACKET_TYPE_GAMUT_METADATA: + lspcon_drm_write_infoframe(encoder, crtc_state, + HDMI_PACKET_TYPE_GAMUT_METADATA, + frame, VIDEO_DIP_DATA_SIZE); + break; + default: + return; + } if (!ret) { - DRM_ERROR("Failed to write AVI infoframes\n"); + DRM_ERROR("Failed to write infoframes\n"); return; } - DRM_DEBUG_DRIVER("AVI infoframes updated successfully\n"); + DRM_DEBUG_DRIVER("Infoframes updated successfully\n"); } void lspcon_read_infoframe(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.h b/drivers/gpu/drm/i915/display/intel_lspcon.h index 1cffe8a42a08..3fac05535731 100644 --- a/drivers/gpu/drm/i915/display/intel_lspcon.h +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h @@ -34,5 +34,8 @@ u32 lspcon_infoframes_enabled(struct intel_encoder *encoder, const struct intel_crtc_state *pipe_config); void lspcon_ycbcr420_config(struct drm_connector *connector, struct intel_crtc_state *crtc_state); - +void lspcon_drm_write_infoframe(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, + unsigned int type, + const void *frame, ssize_t len); #endif /* __INTEL_LSPCON_H__ */
Gen9 hardware supports HDMI2.0 through LSPCON chips. Extending HDR support for MCA LSPCON based GEN9 devices. SOC will drive LSPCON as DP and send HDR metadata as standard DP SDP packets. LSPCON will be set to operate in PCON mode, will receive the metadata and create Dynamic Range and Mastering Infoframe (DRM packets) and send it to HDR capable HDMI sink devices. v2: Re-used hsw infoframe write implementation for HDR metadata for LSPCON as per Ville's suggestion. v3: Addressed Jani Nikula's review comments. Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/i915/display/intel_hdmi.c | 10 ++++++ drivers/gpu/drm/i915/display/intel_lspcon.c | 37 +++++++++++++++------ drivers/gpu/drm/i915/display/intel_lspcon.h | 5 ++- 3 files changed, 40 insertions(+), 12 deletions(-)