>-----Original Message-----
>From: Mun, Gwan-gyeong
>Sent: Tuesday, September 3, 2019 10:54 AM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Sharma, Shashank
><shashank.sharma@xxxxxxxxx>
>Subject: Re: [PATCH v2 5/6] drm/i915/dp: Program an Infoframe SDP Header and DB
>for HDR Static Metadata
>
>On Tue, 2019-08-27 at 01:14 +0530, Shankar, Uma wrote:
>> > -----Original Message-----
>> > From: Mun, Gwan-gyeong
>> > Sent: Friday, August 23, 2019 3:23 PM
>> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Shankar, Uma <
>> > uma.shankar@xxxxxxxxx>; Sharma, Shashank <shashank.sharma@xxxxxxxxx>
>> > Subject: [PATCH v2 5/6] drm/i915/dp: Program an Infoframe SDP Header
>> > and DB for HDR Static Metadata
>> >
>> > Function intel_dp_setup_hdr_metadata_infoframe_sdp handles Infoframe
>> > SDP header and data block setup for HDR Static Metadata. It enables
>> > writing of HDR metadata infoframe SDP to panel. Support for HDR
>> > video was introduced in DisplayPort 1.4. It implements the CTA-861-G
>> > standard for transport of static HDR metadata. The HDR Metadata will
>> > be provided by userspace compositors, based on blending policies and
>> > passed to the driver through a blob property.
>> >
>> > Because each of GEN11 and prior GEN11 have different register size
>> > for HDR Metadata Infoframe SDP packet, it adds and uses different
>> > register size.
>> >
>> > Setup Infoframe SDP header and data block in function
>> > intel_dp_setup_hdr_metadata_infoframe_sdp for HDR Static Metadata as
>> > per dp 1.4 spec and CTA-861-F spec.
>> > As per DP 1.4 spec, 2.2.2.5 SDP Formats. It enables Dynamic Range
>> > and Mastering Infoframe for HDR content, which is defined in
>> > CTA-861-F spec.
>> > According to DP 1.4 spec and CEA-861-F spec Table 5, in order to
>> > transmit static HDR metadata, we have to use Non-audio INFOFRAME SDP
>> > v1.3.
>> >
>> > +--------------------------------+-------------------------------+
>> > > [ Packet Type Value ] | [ Packet Type ] |
>> > +--------------------------------+-------------------------------+
>> > > 80h + Non-audio INFOFRAME Type | CEA-861-F Non-audio INFOFRAME |
>> > +--------------------------------+-------------------------------+
>> > > [Transmission Timing] |
>> > +----------------------------------------------------------------+
>> > > As per CEA-861-F for INFOFRAME, including CEA-861.3 within |
>> > > which Dynamic Range and Mastering INFOFRAME are defined |
>> > +----------------------------------------------------------------+
>> >
>> > v2: Add a missed blank line after function declaration.
>> >
>> > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
>> > drivers/gpu/drm/i915/display/intel_dp.c | 91
>> > ++++++++++++++++++++++++
>> > drivers/gpu/drm/i915/display/intel_dp.h | 3 +
>> > drivers/gpu/drm/i915/i915_reg.h | 1 +
>> > 4 files changed, 96 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
>> > b/drivers/gpu/drm/i915/display/intel_ddi.c
>> > index 5c42b58c1c2f..9f5bea941bcd 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> > @@ -3478,6 +3478,7 @@ static void intel_enable_ddi_dp(struct
>> > intel_encoder *encoder,
>> > intel_edp_backlight_on(crtc_state, conn_state);
>> > intel_psr_enable(intel_dp, crtc_state);
>> > intel_dp_vsc_enable(intel_dp, crtc_state, conn_state);
>> > + intel_dp_hdr_metadata_enable(intel_dp, crtc_state, conn_state);
>> > intel_edp_drrs_enable(intel_dp, crtc_state);
>> >
>> > if (crtc_state->has_audio)
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index 7218e159f55d..00da8221eaba 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -4554,6 +4554,85 @@ intel_dp_setup_vsc_sdp(struct intel_dp
>> > *intel_dp,
>> > crtc_state, DP_SDP_VSC, &vsc_sdp, sizeof(vsc_sdp)); }
>> >
>> > +static int
>> > +intel_dp_setup_hdr_metadata_infoframe_sdp(struct intel_dp
>> > *intel_dp,
>> > + const struct intel_crtc_state
>> > *crtc_state,
>> > + const struct
>> > drm_connector_state
>>
>> The return value is not handled, you may convert it as void.
>>
>Okay, I'll remove the return values which is not handled from
>intel_dp_setup_hdr_metadata_infoframe_sdp().
>
>> > *conn_state) {
>> > + struct intel_digital_port *intel_dig_port =
>> > dp_to_dig_port(intel_dp);
>> > + struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
>> > >base.base.dev);
>> > + struct dp_sdp infoframe_sdp = {};
>> > + struct hdmi_drm_infoframe drm_infoframe = {};
>> > + const int infoframe_size = HDMI_INFOFRAME_HEADER_SIZE +
>> > HDMI_DRM_INFOFRAME_SIZE;
>> > + unsigned char buf[HDMI_INFOFRAME_HEADER_SIZE +
>> > HDMI_DRM_INFOFRAME_SIZE];
>> > + ssize_t len;
>> > + int ret;
>> > +
>> > + ret = drm_hdmi_infoframe_set_hdr_metadata(&drm_infoframe,
>> > conn_state);
>> > + if (ret) {
>> > + DRM_DEBUG_KMS("couldn't set HDR metadata in
>> > infoframe\n");
>> > + return ret;
>> > + }
>> > +
>> > + len = hdmi_drm_infoframe_pack_only(&drm_infoframe, buf,
>> > sizeof(buf));
>> > + if (len < 0) {
>> > + DRM_DEBUG_KMS("buffer size is smaller than hdr metadata
>> > infoframe\n");
>> > + return (int)len;
>>
>> If made void, this will not be required.
>>
>> > + }
>> > +
>> > + if (len != infoframe_size) {
>> > + DRM_DEBUG_KMS("wrong static hdr metadata size\n");
>> > + return -EINVAL;
>> > + }
>> > +
>> > + /*
>> > + * Set up the infoframe sdp packet for HDR static metadata.
>> > + * Prepare VSC Header for SU as per DP 1.4a spec,
>> > + * Table 2-100 and Table 2-101
>> > + */
>> > +
>> > + /* Packet ID, 00h for non-Audio INFOFRAME */
>> > + infoframe_sdp.sdp_header.HB0 = 0;
>> > + /*
>> > + * Packet Type 80h + Non-audio INFOFRAME Type value
>> > + * HDMI_INFOFRAME_TYPE_DRM: 0x87,
>> > + */
>> > + infoframe_sdp.sdp_header.HB1 = drm_infoframe.type;
>> > + /*
>> > + * Least Significant Eight Bits of (Data Byte Count – 1)
>> > + * infoframe_size - 1,
>> > + */
>> > + infoframe_sdp.sdp_header.HB2 = 0x1D;
>> > + /* INFOFRAME SDP Version Number */
>> > + infoframe_sdp.sdp_header.HB3 = (0x13 << 2);
>> > + /* CTA Header Byte 2 (INFOFRAME Version Number) */
>> > + infoframe_sdp.db[0] = drm_infoframe.version;
>> > + /* CTA Header Byte 3 (Length of INFOFRAME):
>> > HDMI_DRM_INFOFRAME_SIZE
>> > */
>> > + infoframe_sdp.db[1] = drm_infoframe.length;
>> > + /*
>> > + * Copy HDMI_DRM_INFOFRAME_SIZE size from a buffer after
>> > + * HDMI_INFOFRAME_HEADER_SIZE
>> > + */
>> > + memcpy(&infoframe_sdp.db[2], &buf[HDMI_INFOFRAME_HEADER_SIZE],
>> > + HDMI_DRM_INFOFRAME_SIZE);
>> > +
>> > + if (INTEL_GEN(dev_priv) >= 11)
>> > + intel_dig_port->write_infoframe(&intel_dig_port->base,
>> > + crtc_state,
>> > +
>> > HDMI_PACKET_TYPE_GAMUT_METADATA,
>> > + &infoframe_sdp,
>> > + VIDEO_DIP_GMP_DATA_SIZE
>> > );
>>
>> This new VIDEO_DIP_GMP_DATA_SIZE doesn't seem to be handled in
>> hsw_write_infoframe (hsw_dip_data_size). Can you please check this.
>>
>Okay, I'll add missed handling of VIDEO_DIP_GMP_DATA_SIZE on
>hsw_dip_data_size().
>> > + else
>> > + /* Prior to GEN11, Header size: 4 bytes, Data size: 28
>> > bytes */
>> > + intel_dig_port->write_infoframe(&intel_dig_port->base,
>> > + crtc_state,
>> > +
>> > HDMI_PACKET_TYPE_GAMUT_METADATA,
>> > + &infoframe_sdp,
>> > + VIDEO_DIP_DATA_SIZE);
>> > +
>>
>> Also can you update the series to handle state checking also for
>> metadata sent to DP sink.
>>
>Does it mean a similar logic of "intel_read_infoframe(encoder, pipe_config,
>HDMI_INFOFRAME_TYPE_DRM, &pipe_config->infoframes.drm);"
>
>if then, because current DP packet related implementation of i915 does not hold DP
>packets (ex. VSC SDP, HDR Metadata Infoframe SDP...) to the pipe state structure. In
>my opinion, as a diffenent series, first, it would be better to add a storing of
>infoframe packets to pipe_state structure for later comparing call (ex.
>intel_pipe_config_compare() ).
>after then we can compare them.
Yeah that’s the idea. We should be having some kind of state checking for DP SDP data as well.
However, we can get this series in and later work on the DP state checking as a separate activity.
>> > + return 0;
>> > +}
>> > +
>> > void intel_dp_vsc_enable(struct intel_dp *intel_dp,
>> > const struct intel_crtc_state *crtc_state,
>> > const struct drm_connector_state *conn_state) @@ -
>> > 4565,6 +4644,18 @@ void intel_dp_vsc_enable(struct intel_dp
>> > *intel_dp,
>> > intel_dp_setup_vsc_sdp(intel_dp, crtc_state, conn_state); }
>> >
>> > +void intel_dp_hdr_metadata_enable(struct intel_dp *intel_dp,
>> > + const struct intel_crtc_state
>> > *crtc_state,
>> > + const struct drm_connector_state
>> > *conn_state) {
>> > + if (!conn_state->hdr_output_metadata)
>> > + return;
>> > +
>> > + intel_dp_setup_hdr_metadata_infoframe_sdp(intel_dp,
>> > + crtc_state,
>> > + conn_state);
>> > +}
>> > +
>> > static u8 intel_dp_autotest_link_training(struct intel_dp
>> > *intel_dp) {
>> > int status = 0;
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
>> > b/drivers/gpu/drm/i915/display/intel_dp.h
>> > index b2da7c9998f7..c3593691dd38 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>> > @@ -115,6 +115,9 @@ bool intel_dp_needs_vsc_colorimetry(u32
>> > colorspace); void
>> > intel_dp_vsc_enable(struct intel_dp *intel_dp,
>> > const struct intel_crtc_state *crtc_state,
>> > const struct drm_connector_state *conn_state);
>> > +void intel_dp_hdr_metadata_enable(struct intel_dp *intel_dp,
>> > + const struct intel_crtc_state
>> > *crtc_state,
>> > + const struct drm_connector_state
>> > *conn_state);
>> > bool intel_digital_port_connected(struct intel_encoder *encoder);
>> >
>> > static inline unsigned int intel_dp_unused_lane_mask(int
>> > lane_count) diff --git
>> > a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index
>> > ea2f0fa2402d..92885416d539 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -4645,6 +4645,7 @@ enum {
>> > * (Haswell and newer) to see which VIDEO_DIP_DATA byte corresponds
>> > to each byte
>> > * of the infoframe structure specified by CEA-861. */
>> > #define VIDEO_DIP_DATA_SIZE 32
>> > +#define VIDEO_DIP_GMP_DATA_SIZE 36
>> > #define VIDEO_DIP_VSC_DATA_SIZE 36
>> > #define VIDEO_DIP_PPS_DATA_SIZE 132
>> > #define VIDEO_DIP_CTL _MMIO(0x61170)
>> > --
>> > 2.22.0
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
