On Mon, Nov 05, 2018 at 05:38:19PM -0800, Srivatsa, Anusha wrote: > > > >-----Original Message----- > >From: Navare, Manasi D > >Sent: Friday, November 2, 2018 2:31 PM > >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx > >Cc: Navare, Manasi D <manasi.d.navare@xxxxxxxxx>; Jani Nikula > ><jani.nikula@xxxxxxxxxxxxxxx>; Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>; > >Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; Harry Wentland > ><harry.wentland@xxxxxxx> > >Subject: [PATCH v8 04/19] drm/dsc: Add helpers for DSC picture parameter set > >infoframes > > > >According to Display Stream compression spec 1.2, the picture parameter set > >metadata is sent from source to sink device using the DP Secondary data packet. > >An infoframe is formed for the PPS SDP header and PPS SDP payload bytes. > >This patch adds helpers to fill the PPS SDP header and PPS SDP payload according > >to the DSC 1.2 specification. > > > >v7: > >* Use BUILD_BUG_ON() to protect changing struct size (Ville) > >* Remove typecaseting (Ville) > >* Include byteorder.h in drm_dsc.c (Ville) > >v6: > >* Use proper sequence points for breaking down the assignments (Chris Wilson) > >* Use SPDX identifier > >v5: > >Do not use bitfields for DRM structs (Jani N) > >v4: > >* Use DSC constants for params that dont change across configurations > >v3: > >* Add reference to added kernel-docs in > >Documentation/gpu/drm-kms-helpers.rst (Daniel Vetter) > > > >v2: > >* Add EXPORT_SYMBOL for the drm functions (Manasi) > > > >Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > >Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > >Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> > >Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> > >Cc: Harry Wentland <harry.wentland@xxxxxxx> > >Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx> > >Acked-by: Harry Wentland <harry.wentland@xxxxxxx> > >--- > > Documentation/gpu/drm-kms-helpers.rst | 12 ++ > > drivers/gpu/drm/Makefile | 2 +- > > drivers/gpu/drm/drm_dsc.c | 228 ++++++++++++++++++++++++++ > > include/drm/drm_dsc.h | 21 +++ > > 4 files changed, 262 insertions(+), 1 deletion(-) create mode 100644 > >drivers/gpu/drm/drm_dsc.c > > > >diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm- > >kms-helpers.rst > >index 4b4dc236ef6f..b422eb8edf16 100644 > >--- a/Documentation/gpu/drm-kms-helpers.rst > >+++ b/Documentation/gpu/drm-kms-helpers.rst > >@@ -232,6 +232,18 @@ MIPI DSI Helper Functions Reference .. kernel-doc:: > >drivers/gpu/drm/drm_mipi_dsi.c > > :export: > > > >+Display Stream Compression Helper Functions Reference > >+===================================================== > >+ > >+.. kernel-doc:: drivers/gpu/drm/drm_dsc.c > >+ :doc: dsc helpers > >+ > >+.. kernel-doc:: include/drm/drm_dsc.h > >+ :internal: > >+ > >+.. kernel-doc:: drivers/gpu/drm/drm_dsc.c > >+ :export: > >+ > > Output Probing Helper Functions Reference > >========================================= > > > >diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index > >576ba985e138..3a3e6fb6d476 100644 > >--- a/drivers/gpu/drm/Makefile > >+++ b/drivers/gpu/drm/Makefile > >@@ -32,7 +32,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o > > drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o > > drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o > > > >-drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \ > >+drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o > >+drm_probe_helper.o \ > > drm_plane_helper.o drm_dp_mst_topology.o > >drm_atomic_helper.o \ > > drm_kms_helper_common.o drm_dp_dual_mode_helper.o \ > > drm_simple_kms_helper.o drm_modeset_helper.o \ diff --git > >a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c new file mode > >100644 index 000000000000..3a4942c1ae3b > >--- /dev/null > >+++ b/drivers/gpu/drm/drm_dsc.c > >@@ -0,0 +1,228 @@ > >+// SPDX-License-Identifier: MIT > > Nit- > In some places the License is within /* */ like comment.... > Is the right way to Add License Identifier? Yes /* */ was giving me checkpatch warning. > > >+/* > >+ * Copyright © 2018 Intel Corp > >+ * > >+ * Author: > >+ * Manasi Navare <manasi.d.navare@xxxxxxxxx> */ > >+ > >+#include <linux/kernel.h> > >+#include <linux/module.h> > >+#include <linux/init.h> > >+#include <linux/errno.h> > >+#include <linux/byteorder/generic.h> > >+#include <drm/drm_dp_helper.h> > >+#include <drm/drm_dsc.h> > >+ > >+/** > >+ * DOC: dsc helpers > >+ * > >+ * These functions contain some common logic and helpers to deal with > >+VESA > >+ * Display Stream Compression standard required for DSC on Display > >+Port/eDP or > >+ * MIPI display interfaces. > >+ */ > >+ > >+/** > >+ * drm_dsc_dp_pps_header_init() - Initializes the PPS Header > >+ * for DisplayPort as per the DP 1.4 spec. > >+ * @pps_sdp: Secondary data packet for DSC Picture Parameter Set */ > >+void drm_dsc_dp_pps_header_init(struct drm_dsc_pps_infoframe *pps_sdp) > >+{ > >+ memset(&pps_sdp->pps_header, 0, sizeof(pps_sdp->pps_header)); > >+ > >+ pps_sdp->pps_header.HB1 = DP_SDP_PPS; > >+ pps_sdp->pps_header.HB2 = > >DP_SDP_PPS_HEADER_PAYLOAD_BYTES_MINUS_1; > >+} > >+EXPORT_SYMBOL(drm_dsc_dp_pps_header_init); > >+ > >+/** > >+ * drm_dsc_pps_infoframe_pack() - Populates the DSC PPS infoframe > >+ * using the DSC configuration parameters in the order expected > >+ * by the DSC Display Sink device. For the DSC, the sink device > >+ * expects the PPS payload in the big endian format for the fields > >+ * that span more than 1 byte. > >+ * > >+ * @pps_sdp: > >+ * Secondary data packet for DSC Picture Parameter Set > >+ * @dsc_cfg: > >+ * DSC Configuration data filled by driver */ void > >+drm_dsc_pps_infoframe_pack(struct drm_dsc_pps_infoframe *pps_sdp, > >+ const struct drm_dsc_config *dsc_cfg) { > >+ int i; > >+ > >+ /*Protect against someone accidently changing struct size */ > Nit - ^^^ Missing space after /* Cool will fix this missing space. Thanks for the review. Manasi > > Other than these nits, the patch looks good. > > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx? > > >+ BUILD_BUG_ON(sizeof(pps_sdp->pps_payload) != > >+ DP_SDP_PPS_HEADER_PAYLOAD_BYTES_MINUS_1 + 1); > >+ > >+ memset(&pps_sdp->pps_payload, 0, sizeof(pps_sdp->pps_payload)); > >+ > >+ /* PPS 0 */ > >+ pps_sdp->pps_payload.dsc_version = > >+ dsc_cfg->dsc_version_minor | > >+ dsc_cfg->dsc_version_major << > >DSC_PPS_VERSION_MAJOR_SHIFT; > >+ > >+ /* PPS 1, 2 is 0 */ > >+ > >+ /* PPS 3 */ > >+ pps_sdp->pps_payload.pps_3 = > >+ dsc_cfg->line_buf_depth | > >+ dsc_cfg->bits_per_component << DSC_PPS_BPC_SHIFT; > >+ > >+ /* PPS 4 */ > >+ pps_sdp->pps_payload.pps_4 = > >+ ((dsc_cfg->bits_per_pixel & DSC_PPS_BPP_HIGH_MASK) >> > >+ DSC_PPS_MSB_SHIFT) | > >+ dsc_cfg->vbr_enable << DSC_PPS_VBR_EN_SHIFT | > >+ dsc_cfg->enable422 << DSC_PPS_SIMPLE422_SHIFT | > >+ dsc_cfg->convert_rgb << DSC_PPS_CONVERT_RGB_SHIFT | > >+ dsc_cfg->block_pred_enable << > >DSC_PPS_BLOCK_PRED_EN_SHIFT; > >+ > >+ /* PPS 5 */ > >+ pps_sdp->pps_payload.bits_per_pixel_low = > >+ (dsc_cfg->bits_per_pixel & DSC_PPS_LSB_MASK); > >+ > >+ /* > >+ * The DSC panel expects the PPS packet to have big endian format > >+ * for data spanning 2 bytes. Use a macro cpu_to_be16() to convert > >+ * to big endian format. If format is little endian, it will swap > >+ * bytes to convert to Big endian else keep it unchanged. > >+ */ > >+ > >+ /* PPS 6, 7 */ > >+ pps_sdp->pps_payload.pic_height = cpu_to_be16(dsc_cfg->pic_height); > >+ > >+ /* PPS 8, 9 */ > >+ pps_sdp->pps_payload.pic_width = cpu_to_be16(dsc_cfg->pic_width); > >+ > >+ /* PPS 10, 11 */ > >+ pps_sdp->pps_payload.slice_height = > >+cpu_to_be16(dsc_cfg->slice_height); > >+ > >+ /* PPS 12, 13 */ > >+ pps_sdp->pps_payload.slice_width = cpu_to_be16(dsc_cfg->slice_width); > >+ > >+ /* PPS 14, 15 */ > >+ pps_sdp->pps_payload.chunk_size = > >+cpu_to_be16(dsc_cfg->slice_chunk_size); > >+ > >+ /* PPS 16 */ > >+ pps_sdp->pps_payload.initial_xmit_delay_high = > >+ ((dsc_cfg->initial_xmit_delay & > >+ DSC_PPS_INIT_XMIT_DELAY_HIGH_MASK) >> > >+ DSC_PPS_MSB_SHIFT); > >+ > >+ /* PPS 17 */ > >+ pps_sdp->pps_payload.initial_xmit_delay_low = > >+ (dsc_cfg->initial_xmit_delay & DSC_PPS_LSB_MASK); > >+ > >+ /* PPS 18, 19 */ > >+ pps_sdp->pps_payload.initial_dec_delay = > >+ cpu_to_be16(dsc_cfg->initial_dec_delay); > >+ > >+ /* PPS 20 is 0 */ > >+ > >+ /* PPS 21 */ > >+ pps_sdp->pps_payload.initial_scale_value = > >+ dsc_cfg->initial_scale_value; > >+ > >+ /* PPS 22, 23 */ > >+ pps_sdp->pps_payload.scale_increment_interval = > >+ cpu_to_be16(dsc_cfg->scale_increment_interval); > >+ > >+ /* PPS 24 */ > >+ pps_sdp->pps_payload.scale_decrement_interval_high = > >+ ((dsc_cfg->scale_decrement_interval & > >+ DSC_PPS_SCALE_DEC_INT_HIGH_MASK) >> > >+ DSC_PPS_MSB_SHIFT); > >+ > >+ /* PPS 25 */ > >+ pps_sdp->pps_payload.scale_decrement_interval_low = > >+ (dsc_cfg->scale_decrement_interval & DSC_PPS_LSB_MASK); > >+ > >+ /* PPS 26[7:0], PPS 27[7:5] RESERVED */ > >+ > >+ /* PPS 27 */ > >+ pps_sdp->pps_payload.first_line_bpg_offset = > >+ dsc_cfg->first_line_bpg_offset; > >+ > >+ /* PPS 28, 29 */ > >+ pps_sdp->pps_payload.nfl_bpg_offset = > >+ cpu_to_be16(dsc_cfg->nfl_bpg_offset); > >+ > >+ /* PPS 30, 31 */ > >+ pps_sdp->pps_payload.slice_bpg_offset = > >+ cpu_to_be16(dsc_cfg->slice_bpg_offset); > >+ > >+ /* PPS 32, 33 */ > >+ pps_sdp->pps_payload.initial_offset = > >+ cpu_to_be16(dsc_cfg->initial_offset); > >+ > >+ /* PPS 34, 35 */ > >+ pps_sdp->pps_payload.final_offset = > >+cpu_to_be16(dsc_cfg->final_offset); > >+ > >+ /* PPS 36 */ > >+ pps_sdp->pps_payload.flatness_min_qp = dsc_cfg->flatness_min_qp; > >+ > >+ /* PPS 37 */ > >+ pps_sdp->pps_payload.flatness_max_qp = dsc_cfg->flatness_max_qp; > >+ > >+ /* PPS 38, 39 */ > >+ pps_sdp->pps_payload.rc_model_size = > >+ cpu_to_be16(DSC_RC_MODEL_SIZE_CONST); > >+ > >+ /* PPS 40 */ > >+ pps_sdp->pps_payload.rc_edge_factor = > >DSC_RC_EDGE_FACTOR_CONST; > >+ > >+ /* PPS 41 */ > >+ pps_sdp->pps_payload.rc_quant_incr_limit0 = > >+ dsc_cfg->rc_quant_incr_limit0; > >+ > >+ /* PPS 42 */ > >+ pps_sdp->pps_payload.rc_quant_incr_limit1 = > >+ dsc_cfg->rc_quant_incr_limit1; > >+ > >+ /* PPS 43 */ > >+ pps_sdp->pps_payload.rc_tgt_offset = DSC_RC_TGT_OFFSET_LO_CONST > >| > >+ DSC_RC_TGT_OFFSET_HI_CONST << > >DSC_PPS_RC_TGT_OFFSET_HI_SHIFT; > >+ > >+ /* PPS 44 - 57 */ > >+ for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) > >+ pps_sdp->pps_payload.rc_buf_thresh[i] = > >+ dsc_cfg->rc_buf_thresh[i]; > >+ > >+ /* PPS 58 - 87 */ > >+ /* > >+ * For DSC sink programming the RC Range parameter fields > >+ * are as follows: Min_qp[15:11], max_qp[10:6], offset[5:0] > >+ */ > >+ for (i = 0; i < DSC_NUM_BUF_RANGES; i++) { > >+ pps_sdp->pps_payload.rc_range_parameters[i] = > >+ ((dsc_cfg->rc_range_params[i].range_min_qp << > >+ DSC_PPS_RC_RANGE_MINQP_SHIFT) | > >+ (dsc_cfg->rc_range_params[i].range_max_qp << > >+ DSC_PPS_RC_RANGE_MAXQP_SHIFT) | > >+ (dsc_cfg->rc_range_params[i].range_bpg_offset)); > >+ pps_sdp->pps_payload.rc_range_parameters[i] = > >+ cpu_to_be16(pps_sdp- > >>pps_payload.rc_range_parameters[i]); > >+ } > >+ > >+ /* PPS 88 */ > >+ pps_sdp->pps_payload.native_422_420 = dsc_cfg->native_422 | > >+ dsc_cfg->native_420 << DSC_PPS_NATIVE_420_SHIFT; > >+ > >+ /* PPS 89 */ > >+ pps_sdp->pps_payload.second_line_bpg_offset = > >+ dsc_cfg->second_line_bpg_offset; > >+ > >+ /* PPS 90, 91 */ > >+ pps_sdp->pps_payload.nsl_bpg_offset = > >+ cpu_to_be16(dsc_cfg->nsl_bpg_offset); > >+ > >+ /* PPS 92, 93 */ > >+ pps_sdp->pps_payload.second_line_offset_adj = > >+ cpu_to_be16(dsc_cfg->second_line_offset_adj); > >+ > >+ /* PPS 94 - 127 are O */ > >+} > >+EXPORT_SYMBOL(drm_dsc_pps_infoframe_pack); > >diff --git a/include/drm/drm_dsc.h b/include/drm/drm_dsc.h index > >b88e31bd9da7..52e57ceaff80 100644 > >--- a/include/drm/drm_dsc.h > >+++ b/include/drm/drm_dsc.h > >@@ -24,6 +24,23 @@ > > #define DSC_RC_TGT_OFFSET_HI_CONST 3 > > #define DSC_RC_TGT_OFFSET_LO_CONST 3 > > > >+/* DSC PPS constants and macros */ > >+#define DSC_PPS_VERSION_MAJOR_SHIFT 4 > >+#define DSC_PPS_BPC_SHIFT 4 > >+#define DSC_PPS_MSB_SHIFT 8 > >+#define DSC_PPS_LSB_MASK (0xFF << 0) > >+#define DSC_PPS_BPP_HIGH_MASK (0x3 << 8) > >+#define DSC_PPS_VBR_EN_SHIFT 2 > >+#define DSC_PPS_SIMPLE422_SHIFT 3 > >+#define DSC_PPS_CONVERT_RGB_SHIFT 4 > >+#define DSC_PPS_BLOCK_PRED_EN_SHIFT 5 > >+#define DSC_PPS_INIT_XMIT_DELAY_HIGH_MASK (0x3 << 8) > >+#define DSC_PPS_SCALE_DEC_INT_HIGH_MASK (0xF << 8) > >+#define DSC_PPS_RC_TGT_OFFSET_HI_SHIFT 4 > >+#define DSC_PPS_RC_RANGE_MINQP_SHIFT 11 > >+#define DSC_PPS_RC_RANGE_MAXQP_SHIFT 6 > >+#define DSC_PPS_NATIVE_420_SHIFT 1 > >+ > > /* Configuration for a single Rate Control model range */ struct > >drm_dsc_rc_range_parameters { > > /* Min Quantization Parameters allowed for this range */ @@ -458,4 > >+475,8 @@ struct drm_dsc_pps_infoframe { > > struct drm_dsc_picture_parameter_set pps_payload; } __packed; > > > >+void drm_dsc_dp_pps_header_init(struct drm_dsc_pps_infoframe *pps_sdp); > >+void drm_dsc_pps_infoframe_pack(struct drm_dsc_pps_infoframe *pps_sdp, > >+ const struct drm_dsc_config *dsc_cfg); > >+ > > #endif /* _DRM_DSC_H_ */ > >-- > >2.18.0 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel