RE: [PATCH v7 04/18] drm/i915/dp: Add writing of DP SDPs
On Mon, 16 Mar 2020, "Shankar, Uma" wrote:
>> -Original Message-
>> From: dri-devel On Behalf Of Gwan-
>> gyeong Mun
>> Sent: Tuesday, February 11, 2020 1:17 PM
>> To: intel-...@lists.freedesktop.org
>> Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org
>> Subject: [PATCH v7 04/18] drm/i915/dp: Add writing of DP SDPs
>>
>> It adds routines that write DP VSC SDP and DP HDR Metadata Infoframe SDP.
>> In order to pack DP VSC SDP, it adds intel_dp_vsc_sdp_pack() function.
>> It follows DP 1.4a spec. [Table 2-116: VSC SDP Header Bytes] and [Table
>> 2-117: VSC
>> SDP Payload for DB16 through DB18]
>>
>> In order to pack DP HDR Metadata Infoframe SDP, it adds
>> intel_dp_hdr_metadata_infoframe_sdp_pack() function.
>> And it follows DP 1.4a spec.
>> ([Table 2-125: INFOFRAME SDP v1.2 Header Bytes] and [Table 2-126: INFOFRAME
>> SDP v1.2 Payload Data Bytes - DB0 through DB31]) and CTA-861-G spec.
>> [Table-42
>> Dynamic Range and Mastering InfoFrame].
>>
>> A mechanism and a naming rule of intel_dp_set_infoframes() function
>> references
>> intel_encoder->set_infoframes() of intel_hdmi.c .
>> VSC SDP is used for PSR and Pixel Encoding and Colorimetry Formats cases.
>> Because PSR routine has its own routine of writing a VSC SDP, when the PSR is
>> enabled, intel_dp_set_infoframes() does not write a VSC SDP.
>>
>> v3:
>> - Explicitly disable unused DIPs (AVI, GCP, VS, SPD, DRM. They will be
>> used for HDMI), when intel_dp_set_infoframes() function will be called.
>> - Replace a structure name to drm_dp_vsc_sdp from intel_dp_vsc_sdp.
>> v4: Use struct drm_device logging macros
>> v5:
>> - use intel_de_*() functions for register access
>> - Addressed review comments from Uma
>> Polish commit message and comments
>> Add 6bpc to packing of VSC SDP
>
> Looks good to me.
> Reviewed-by: Uma Shankar
Pushed up to and including this patch, thanks for the patches and
review.
BR,
Jani.
>
>> Signed-off-by: Gwan-gyeong Mun
>> ---
>> drivers/gpu/drm/i915/display/intel_dp.c | 199
>> drivers/gpu/drm/i915/display/intel_dp.h | 3 +
>> 2 files changed, 202 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index fb008168ca83..5bbc55113325 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4741,6 +4741,205 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state
>> *crtc_state,
>> return false;
>> }
>>
>> +static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
>> + struct dp_sdp *sdp, size_t size) {
>> +size_t length = sizeof(struct dp_sdp);
>> +
>> +if (size < length)
>> +return -ENOSPC;
>> +
>> +memset(sdp, 0, size);
>> +
>> +/*
>> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
>> + * VSC SDP Header Bytes
>> + */
>> +sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
>> +sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
>> +sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
>> +sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
>> +
>> +/* VSC SDP Payload for DB16 through DB18 */
>> +/* Pixel Encoding and Colorimetry Formats */
>> +sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
>> +sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
>> +
>> +switch (vsc->bpc) {
>> +case 6:
>> +/* 6bpc: 0x0 */
>> +break;
>> +case 8:
>> +sdp->db[17] = 0x1; /* DB17[3:0] */
>> +break;
>> +case 10:
>> +sdp->db[17] = 0x2;
>> +break;
>> +case 12:
>> +sdp->db[17] = 0x3;
>> +break;
>> +case 16:
>> +sdp->db[17] = 0x4;
>> +break;
>> +default:
>> +MISSING_CASE(vsc->bpc);
>> +break;
>> +}
>> +/* Dynamic Range and Component Bit Depth */
>> +if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
>> +sdp->db[17] |= 0x80; /* DB17[7] */
>> +
>> +/* Content Type */
>> +sdp->db[18] = vsc->content_type & 0x7;
>> +
>> +return length;
>> +}
>> +
>> +static ssize_t
>> +intel_dp_hdr_metadata_infoframe_sdp_pack(const struct hdmi_drm_infoframe
>> *drm_infoframe,
>> + struct dp_sdp *sdp,
>> + size_t size)
>> +{
>> +size_t length = sizeof(struct dp_sdp);
>> +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;
>> +
>> +if (size < length)
>> +return -ENOSPC;
>> +
>> +memset(sdp, 0, size);
>> +
>> +len = hdmi_drm_infoframe_pack_only(drm_infoframe, buf, sizeof(buf));
>> +if (len < 0) {
>> +
RE: [PATCH v7 04/18] drm/i915/dp: Add writing of DP SDPs
> -Original Message-
> From: dri-devel On Behalf Of Gwan-
> gyeong Mun
> Sent: Tuesday, February 11, 2020 1:17 PM
> To: intel-...@lists.freedesktop.org
> Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org
> Subject: [PATCH v7 04/18] drm/i915/dp: Add writing of DP SDPs
>
> It adds routines that write DP VSC SDP and DP HDR Metadata Infoframe SDP.
> In order to pack DP VSC SDP, it adds intel_dp_vsc_sdp_pack() function.
> It follows DP 1.4a spec. [Table 2-116: VSC SDP Header Bytes] and [Table
> 2-117: VSC
> SDP Payload for DB16 through DB18]
>
> In order to pack DP HDR Metadata Infoframe SDP, it adds
> intel_dp_hdr_metadata_infoframe_sdp_pack() function.
> And it follows DP 1.4a spec.
> ([Table 2-125: INFOFRAME SDP v1.2 Header Bytes] and [Table 2-126: INFOFRAME
> SDP v1.2 Payload Data Bytes - DB0 through DB31]) and CTA-861-G spec. [Table-42
> Dynamic Range and Mastering InfoFrame].
>
> A mechanism and a naming rule of intel_dp_set_infoframes() function references
> intel_encoder->set_infoframes() of intel_hdmi.c .
> VSC SDP is used for PSR and Pixel Encoding and Colorimetry Formats cases.
> Because PSR routine has its own routine of writing a VSC SDP, when the PSR is
> enabled, intel_dp_set_infoframes() does not write a VSC SDP.
>
> v3:
> - Explicitly disable unused DIPs (AVI, GCP, VS, SPD, DRM. They will be
> used for HDMI), when intel_dp_set_infoframes() function will be called.
> - Replace a structure name to drm_dp_vsc_sdp from intel_dp_vsc_sdp.
> v4: Use struct drm_device logging macros
> v5:
> - use intel_de_*() functions for register access
> - Addressed review comments from Uma
> Polish commit message and comments
> Add 6bpc to packing of VSC SDP
Looks good to me.
Reviewed-by: Uma Shankar
> Signed-off-by: Gwan-gyeong Mun
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 199
> drivers/gpu/drm/i915/display/intel_dp.h | 3 +
> 2 files changed, 202 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index fb008168ca83..5bbc55113325 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4741,6 +4741,205 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state
> *crtc_state,
> return false;
> }
>
> +static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> + struct dp_sdp *sdp, size_t size) {
> + size_t length = sizeof(struct dp_sdp);
> +
> + if (size < length)
> + return -ENOSPC;
> +
> + memset(sdp, 0, size);
> +
> + /*
> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> + * VSC SDP Header Bytes
> + */
> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> +
> + /* VSC SDP Payload for DB16 through DB18 */
> + /* Pixel Encoding and Colorimetry Formats */
> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> +
> + switch (vsc->bpc) {
> + case 6:
> + /* 6bpc: 0x0 */
> + break;
> + case 8:
> + sdp->db[17] = 0x1; /* DB17[3:0] */
> + break;
> + case 10:
> + sdp->db[17] = 0x2;
> + break;
> + case 12:
> + sdp->db[17] = 0x3;
> + break;
> + case 16:
> + sdp->db[17] = 0x4;
> + break;
> + default:
> + MISSING_CASE(vsc->bpc);
> + break;
> + }
> + /* Dynamic Range and Component Bit Depth */
> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> + sdp->db[17] |= 0x80; /* DB17[7] */
> +
> + /* Content Type */
> + sdp->db[18] = vsc->content_type & 0x7;
> +
> + return length;
> +}
> +
> +static ssize_t
> +intel_dp_hdr_metadata_infoframe_sdp_pack(const struct hdmi_drm_infoframe
> *drm_infoframe,
> + struct dp_sdp *sdp,
> + size_t size)
> +{
> + size_t length = sizeof(struct dp_sdp);
> + 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;
> +
> + if (size < length)
> + return -ENOSPC;
> +
> + memset(sdp, 0, size);
> +
> + 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 -ENOSPC;
> + }
> +
> + if (len != infoframe_size) {
> + DRM_DEBUG_KMS("wrong static hdr metadata size\n");
> +
