Re: [PATCH 3/3] drm/dsc: Change infoframe_pack to payload_pack
On Wed, Feb 13, 2019 at 09:45:36AM -0500, 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)
>
I think if we are just sending pps_payload as an argument to this function
to pack payload, it also makes sense to just send pps_header as an input
to drm_dsc_dp_pps_header_init() to follow the consistency there.
So with that the caller will have to call the header_init first , initialize the
sdp header and then call the _payload_pack to pack the payload bytes.
And then send the entire infoframe to the sink.
Could you please also make that change in this patch?
Regards
Manasi
> Signed-off-by: David Francis
> ---
> 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(_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 */
> -
Re: [PATCH 2/3] drm/dsc: Add native 420 and 422 support to compute_rc_params
On Wed, Feb 13, 2019 at 09:45:35AM -0500, David Francis wrote:
> Native 420 and 422 transfer modes are new in DSC1.2
>
> In these modes, each two pixels of a slice are treated as one
> pixel, so the slice width is half as large (round down) for
> the purposes of calucating the groups per line and chunk size
> in bytes
>
> In native 422 mode, each pixel has four components, so the
> mux component of a group is larger by one additional mux word
> and one additional component
>
> Now that there is native 422 support, the configuration option
> previously called enable422 is renamed to simple_422 to avoid
> confusion
>
> Signed-off-by: David Francis
This looks good and verified that the DSC 1.2 spec actually renames it
as simple_422.
Reviewed-by: Manasi Navare
Manasi
> ---
> drivers/gpu/drm/drm_dsc.c | 31 +++
> drivers/gpu/drm/i915/intel_vdsc.c | 4 ++--
> include/drm/drm_dsc.h | 4 ++--
> 3 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c
> index 4b0e3c9c3ff8..9e675dd39a44 100644
> --- a/drivers/gpu/drm/drm_dsc.c
> +++ b/drivers/gpu/drm/drm_dsc.c
> @@ -77,7 +77,7 @@ void drm_dsc_pps_infoframe_pack(struct
> drm_dsc_pps_infoframe *pps_sdp,
> ((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->simple_422 << DSC_PPS_SIMPLE422_SHIFT |
> dsc_cfg->convert_rgb << DSC_PPS_CONVERT_RGB_SHIFT |
> dsc_cfg->block_pred_enable << DSC_PPS_BLOCK_PRED_EN_SHIFT;
>
> @@ -246,19 +246,34 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
> *vdsc_cfg)
> unsigned long final_scale = 0;
> unsigned long rbs_min = 0;
>
> - /* Number of groups used to code each line of a slice */
> - groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> -DSC_RC_PIXELS_PER_GROUP);
> + if (vdsc_cfg->native_420 || vdsc_cfg->native_422) {
> + /* Number of groups used to code each line of a slice */
> + groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width / 2,
> +DSC_RC_PIXELS_PER_GROUP);
>
> - /* chunksize in Bytes */
> - vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
> - vdsc_cfg->bits_per_pixel,
> - (8 * 16));
> + /* chunksize in Bytes */
> + vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width
> / 2 *
> +
> vdsc_cfg->bits_per_pixel,
> + (8 * 16));
> + } else {
> + /* Number of groups used to code each line of a slice */
> + groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> +DSC_RC_PIXELS_PER_GROUP);
> +
> + /* chunksize in Bytes */
> + vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width
> *
> +
> vdsc_cfg->bits_per_pixel,
> + (8 * 16));
> + }
>
> if (vdsc_cfg->convert_rgb)
> num_extra_mux_bits = 3 * (vdsc_cfg->mux_word_size +
> (4 * vdsc_cfg->bits_per_component + 4)
> - 2);
> + else if (vdsc_cfg->native_422)
> + num_extra_mux_bits = 4 * vdsc_cfg->mux_word_size +
> + (4 * vdsc_cfg->bits_per_component + 4) +
> + 3 * (4 * vdsc_cfg->bits_per_component) - 2;
> else
> num_extra_mux_bits = 3 * vdsc_cfg->mux_word_size +
> (4 * vdsc_cfg->bits_per_component + 4) +
> diff --git a/drivers/gpu/drm/i915/intel_vdsc.c
> b/drivers/gpu/drm/i915/intel_vdsc.c
> index c76cec8bfb74..7702c5c8b3f2 100644
> --- a/drivers/gpu/drm/i915/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> @@ -369,7 +369,7 @@ int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> DSC_1_1_MAX_LINEBUF_DEPTH_BITS : line_buf_depth;
>
> /* Gen 11 does not support YCbCr */
> - vdsc_cfg->enable422 = false;
> + vdsc_cfg->simple_422 = false;
> /* Gen 11 does not support VBR */
> vdsc_cfg->vbr_enable = false;
> vdsc_cfg->block_pred_enable =
> @@ -496,7 +496,7 @@ static void intel_configure_pps_for_dsc_encoder(struct
> intel_encoder *encoder,
> pps_val |= DSC_BLOCK_PREDICTION;
> if (vdsc_cfg->convert_rgb)
> pps_val |= DSC_COLOR_SPACE_CONVERSION;
> - if (vdsc_cfg->enable422)
> + if (vdsc_cfg->simple_422)
>
Re: [PATCH 1/3] drm/i915: Move dsc rate params compute into drm
On Wed, Feb 13, 2019 at 09:45:34AM -0500, David Francis wrote:
> The function intel_compute_rc_parameters is part of the dsc spec
> and is not driver-specific. Other drm drivers might like to use
> it. The function is not changed; just moved and renamed.
>
Yes this sounds fair since its DSC spec related and can move to drm_dsc.c.
As a part of this series or later you should also consider moving the
rc_parameters struct for input bpc/output BPP combinations to DRM since that
is also purely spec related.
With this change and compute_rc_params function in DRM, please add appropriate
description of the function as part of kernel documentation.
With the documentation change, you have my r-b.
Regards
Manasi
> Signed-off-by: David Francis
> ---
> drivers/gpu/drm/drm_dsc.c | 133 ++
> drivers/gpu/drm/i915/intel_vdsc.c | 125 +---
> include/drm/drm_dsc.h | 1 +
> 3 files changed, 135 insertions(+), 124 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c
> index bc2b23adb072..4b0e3c9c3ff8 100644
> --- a/drivers/gpu/drm/drm_dsc.c
> +++ b/drivers/gpu/drm/drm_dsc.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -226,3 +227,135 @@ void drm_dsc_pps_infoframe_pack(struct
> drm_dsc_pps_infoframe *pps_sdp,
> /* PPS 94 - 127 are O */
> }
> EXPORT_SYMBOL(drm_dsc_pps_infoframe_pack);
> +
> +/**
> + * drm_dsc_compute_rc_parameters() - Write rate control
> + * parameters to the dsc configuration. Some configuration
> + * fields must be present beforehand.
> + *
> + * @dsc_cfg:
> + * DSC Configuration data partially filled by driver
> + */
> +int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
> +{
> + unsigned long groups_per_line = 0;
> + unsigned long groups_total = 0;
> + unsigned long num_extra_mux_bits = 0;
> + unsigned long slice_bits = 0;
> + unsigned long hrd_delay = 0;
> + unsigned long final_scale = 0;
> + unsigned long rbs_min = 0;
> +
> + /* Number of groups used to code each line of a slice */
> + groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> +DSC_RC_PIXELS_PER_GROUP);
> +
> + /* chunksize in Bytes */
> + vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
> + vdsc_cfg->bits_per_pixel,
> + (8 * 16));
> +
> + if (vdsc_cfg->convert_rgb)
> + num_extra_mux_bits = 3 * (vdsc_cfg->mux_word_size +
> + (4 * vdsc_cfg->bits_per_component + 4)
> + - 2);
> + else
> + num_extra_mux_bits = 3 * vdsc_cfg->mux_word_size +
> + (4 * vdsc_cfg->bits_per_component + 4) +
> + 2 * (4 * vdsc_cfg->bits_per_component) - 2;
> + /* Number of bits in one Slice */
> + slice_bits = 8 * vdsc_cfg->slice_chunk_size * vdsc_cfg->slice_height;
> +
> + while ((num_extra_mux_bits > 0) &&
> +((slice_bits - num_extra_mux_bits) % vdsc_cfg->mux_word_size))
> + num_extra_mux_bits--;
> +
> + if (groups_per_line < vdsc_cfg->initial_scale_value - 8)
> + vdsc_cfg->initial_scale_value = groups_per_line + 8;
> +
> + /* scale_decrement_interval calculation according to DSC spec 1.11 */
> + if (vdsc_cfg->initial_scale_value > 8)
> + vdsc_cfg->scale_decrement_interval = groups_per_line /
> + (vdsc_cfg->initial_scale_value - 8);
> + else
> + vdsc_cfg->scale_decrement_interval =
> DSC_SCALE_DECREMENT_INTERVAL_MAX;
> +
> + vdsc_cfg->final_offset = vdsc_cfg->rc_model_size -
> + (vdsc_cfg->initial_xmit_delay *
> + vdsc_cfg->bits_per_pixel + 8) / 16 + num_extra_mux_bits;
> +
> + if (vdsc_cfg->final_offset >= vdsc_cfg->rc_model_size) {
> + DRM_DEBUG_KMS("FinalOfs < RcModelSze for this
> InitialXmitDelay\n");
> + return -ERANGE;
> + }
> +
> + final_scale = (vdsc_cfg->rc_model_size * 8) /
> + (vdsc_cfg->rc_model_size - vdsc_cfg->final_offset);
> + if (vdsc_cfg->slice_height > 1)
> + /*
> + * NflBpgOffset is 16 bit value with 11 fractional bits
> + * hence we multiply by 2^11 for preserving the
> + * fractional part
> + */
> + vdsc_cfg->nfl_bpg_offset =
> DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset << 11),
> + (vdsc_cfg->slice_height
> - 1));
> + else
> + vdsc_cfg->nfl_bpg_offset = 0;
> +
> + /* 2^16 - 1 */
> + if (vdsc_cfg->nfl_bpg_offset > 65535) {
> + DRM_DEBUG_KMS("NflBpgOffset is too large for this slice
> height\n");
> + return -ERANGE;
> + }
> +
> + /* Number of
