Re: [PATCH 3/3] drm/dsc: Change infoframe_pack to payload_pack

["Wentland, Harry" <Harry.Wentland@amd.com>]

On 2019-02-13 9:45 a.m., David Francis wrote:
> The function drm_dsc_pps_infoframe_pack only
> packed the payload portion of the infoframe.
> Change the input struct to the PPS payload
> to clarify the function's purpose and allow
> for drivers with their own handling of sdp.
> (e.g. drivers with their own struct for
> all SDP transactions)
> 
> Signed-off-by: David Francis <David.Francis@amd.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Again, ideally we'd want an AB from i915 guys as well.

Harry

> ---
>  drivers/gpu/drm/drm_dsc.c         | 86 +++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_vdsc.c |  2 +-
>  include/drm/drm_dsc.h             |  2 +-
>  3 files changed, 45 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c
> index 9e675dd39a44..4ada4d4f59ac 100644
> --- a/drivers/gpu/drm/drm_dsc.c
> +++ b/drivers/gpu/drm/drm_dsc.c
> @@ -38,42 +38,42 @@ void drm_dsc_dp_pps_header_init(struct drm_dsc_pps_infoframe *pps_sdp)
>  EXPORT_SYMBOL(drm_dsc_dp_pps_header_init);
>  
>  /**
> - * drm_dsc_pps_infoframe_pack() - Populates the DSC PPS infoframe
> + * drm_dsc_pps_payload_pack() - Populates the DSC PPS payload
>   * 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
> + * @pps_payload:
> + * 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,
> +void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>  				const struct drm_dsc_config *dsc_cfg)
>  {
>  	int i;
>  
>  	/* Protect against someone accidently changing struct size */
> -	BUILD_BUG_ON(sizeof(pps_sdp->pps_payload) !=
> +	BUILD_BUG_ON(sizeof(*pps_payload) !=
>  		     DP_SDP_PPS_HEADER_PAYLOAD_BYTES_MINUS_1 + 1);
>  
> -	memset(&pps_sdp->pps_payload, 0, sizeof(pps_sdp->pps_payload));
> +	memset(pps_payload, 0, sizeof(*pps_payload));
>  
>  	/* PPS 0 */
> -	pps_sdp->pps_payload.dsc_version =
> +	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 =
> +	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 =
> +	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 |
> @@ -82,7 +82,7 @@ void drm_dsc_pps_infoframe_pack(struct drm_dsc_pps_infoframe *pps_sdp,
>  		dsc_cfg->block_pred_enable << DSC_PPS_BLOCK_PRED_EN_SHIFT;
>  
>  	/* PPS 5 */
> -	pps_sdp->pps_payload.bits_per_pixel_low =
> +	pps_payload->bits_per_pixel_low =
>  		(dsc_cfg->bits_per_pixel & DSC_PPS_LSB_MASK);
>  
>  	/*
> @@ -93,103 +93,103 @@ void drm_dsc_pps_infoframe_pack(struct drm_dsc_pps_infoframe *pps_sdp,
>  	 */
>  
>  	/* PPS 6, 7 */
> -	pps_sdp->pps_payload.pic_height = cpu_to_be16(dsc_cfg->pic_height);
> +	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_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_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_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_payload->chunk_size = cpu_to_be16(dsc_cfg->slice_chunk_size);
>  
>  	/* PPS 16 */
> -	pps_sdp->pps_payload.initial_xmit_delay_high =
> +	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 =
> +	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 =
> +	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 =
> +	pps_payload->initial_scale_value =
>  		dsc_cfg->initial_scale_value;
>  
>  	/* PPS 22, 23 */
> -	pps_sdp->pps_payload.scale_increment_interval =
> +	pps_payload->scale_increment_interval =
>  		cpu_to_be16(dsc_cfg->scale_increment_interval);
>  
>  	/* PPS 24 */
> -	pps_sdp->pps_payload.scale_decrement_interval_high =
> +	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 =
> +	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 =
> +	pps_payload->first_line_bpg_offset =
>  		dsc_cfg->first_line_bpg_offset;
>  
>  	/* PPS 28, 29 */
> -	pps_sdp->pps_payload.nfl_bpg_offset =
> +	pps_payload->nfl_bpg_offset =
>  		cpu_to_be16(dsc_cfg->nfl_bpg_offset);
>  
>  	/* PPS 30, 31 */
> -	pps_sdp->pps_payload.slice_bpg_offset =
> +	pps_payload->slice_bpg_offset =
>  		cpu_to_be16(dsc_cfg->slice_bpg_offset);
>  
>  	/* PPS 32, 33 */
> -	pps_sdp->pps_payload.initial_offset =
> +	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_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_payload->flatness_min_qp = dsc_cfg->flatness_min_qp;
>  
>  	/* PPS 37 */
> -	pps_sdp->pps_payload.flatness_max_qp = dsc_cfg->flatness_max_qp;
> +	pps_payload->flatness_max_qp = dsc_cfg->flatness_max_qp;
>  
>  	/* PPS 38, 39 */
> -	pps_sdp->pps_payload.rc_model_size =
> +	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_payload->rc_edge_factor = DSC_RC_EDGE_FACTOR_CONST;
>  
>  	/* PPS 41 */
> -	pps_sdp->pps_payload.rc_quant_incr_limit0 =
> +	pps_payload->rc_quant_incr_limit0 =
>  		dsc_cfg->rc_quant_incr_limit0;
>  
>  	/* PPS 42 */
> -	pps_sdp->pps_payload.rc_quant_incr_limit1 =
> +	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 |
> +	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] =
> +		pps_payload->rc_buf_thresh[i] =
>  			dsc_cfg->rc_buf_thresh[i];
>  
>  	/* PPS 58 - 87 */
> @@ -198,35 +198,35 @@ void drm_dsc_pps_infoframe_pack(struct drm_dsc_pps_infoframe *pps_sdp,
>  	 * 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] =
> +		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_payload->rc_range_parameters[i] =
> +			cpu_to_be16(pps_payload->rc_range_parameters[i]);
>  	}
>  
>  	/* PPS 88 */
> -	pps_sdp->pps_payload.native_422_420 = dsc_cfg->native_422 |
> +	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 =
> +	pps_payload->second_line_bpg_offset =
>  		dsc_cfg->second_line_bpg_offset;
>  
>  	/* PPS 90, 91 */
> -	pps_sdp->pps_payload.nsl_bpg_offset =
> +	pps_payload->nsl_bpg_offset =
>  		cpu_to_be16(dsc_cfg->nsl_bpg_offset);
>  
>  	/* PPS 92, 93 */
> -	pps_sdp->pps_payload.second_line_offset_adj =
> +	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);
> +EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>  
>  /**
>   * drm_dsc_compute_rc_parameters() - Write rate control
> diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
> index 7702c5c8b3f2..223c7b6c6e78 100644
> --- a/drivers/gpu/drm/i915/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> @@ -885,7 +885,7 @@ static void intel_dp_write_dsc_pps_sdp(struct intel_encoder *encoder,
>  	drm_dsc_dp_pps_header_init(&dp_dsc_pps_sdp);
>  
>  	/* Fill the PPS payload bytes as per DSC spec 1.2 Table 4-1 */
> -	drm_dsc_pps_infoframe_pack(&dp_dsc_pps_sdp, vdsc_cfg);
> +	drm_dsc_pps_payload_pack(&dp_dsc_pps_sdp.pps_payload, vdsc_cfg);
>  
>  	intel_dig_port->write_infoframe(encoder, crtc_state,
>  					DP_SDP_PPS, &dp_dsc_pps_sdp,
> diff --git a/include/drm/drm_dsc.h b/include/drm/drm_dsc.h
> index 4e55e37943d7..3bed1f119576 100644
> --- a/include/drm/drm_dsc.h
> +++ b/include/drm/drm_dsc.h
> @@ -479,7 +479,7 @@ struct drm_dsc_pps_infoframe {
>  } __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,
> +void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
>  				const struct drm_dsc_config *dsc_cfg);
>  int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>  
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx