Re: [PATCH v5 17/28] drm/i915/dsc: Compute Rate Control parameters for DSC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: Navare, Manasi D
>Sent: Tuesday, October 23, 2018 11:43 AM
>To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
>Subject: Re:  [PATCH v5 17/28] drm/i915/dsc: Compute Rate Control
>parameters for DSC
>
>On Mon, Oct 22, 2018 at 04:34:59PM -0700, Srivatsa, Anusha wrote:
>>
>> ________________________________________
>> From: Intel-gfx [intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] on behalf of
>> Manasi Navare [manasi.d.navare@xxxxxxxxx]
>> Sent: Friday, October 05, 2018 4:22 PM
>> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> Subject:  [PATCH v5 17/28] drm/i915/dsc: Compute Rate
>> Control parameters for DSC
>>
>> From: Gaurav K Singh <gaurav.k.singh@xxxxxxxxx>
>>
>> This computation of RC params happens in the atomic commit phase
>> during compute_config() to validate if display stream compression can
>> be enabled for the requested mode.
>>
>> v5 (From Manasi):
>> * Fix dim checkpatch warnings/checks
>> v4(From Gaurav):
>> * No change.Rebase on drm-tip
>>
>> v3 (From Gaurav):
>> * Rebase on top of Manasi's latest series
>> * Return -ve value in case of failure scenarios (Manasi)
>>
>> Fix review comments from Ville:
>> * Remove unnecessary comments
>> * Remove unnecessary paranthesis
>> * Add comments for few RC params calculations
>>
>> v2 (From Manasi):
>> * Rebase Gaurav's patch from intel-gfx to gfx-internal
>> * Use struct drm_dsc_cfg instead of struct intel_dp as a parameter
>>
>> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
>> Signed-off-by: Gaurav K Singh <gaurav.k.singh@xxxxxxxxx>
>> Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/intel_vdsc.c | 127
>> ++++++++++++++++++++++++++++++
>>  1 file changed, 127 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_vdsc.c
>> b/drivers/gpu/drm/i915/intel_vdsc.c
>> index 351ea7d71c21..594196a9d0f4 100644
>> --- a/drivers/gpu/drm/i915/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/intel_vdsc.c
>> @@ -317,6 +317,130 @@ static int get_column_index_for_rc_params(u8
>bits_per_component)
>>         }
>>  }
>>
>> +static int intel_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_ERROR("FinalOfs < RcModelSze for this InitialXmitDelay\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       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_ERROR("NflBpgOffset is too large for this slice height\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Number of groups used to code the entire slice */
>> +       groups_total = groups_per_line * vdsc_cfg->slice_height;
>> +
>> +       /* slice_bpg_offset is 16 bit value with 11 fractional bits */
>> +       vdsc_cfg->slice_bpg_offset = DIV_ROUND_UP(((vdsc_cfg->rc_model_size
>-
>> +                                                   vdsc_cfg->initial_offset +
>> +                                                   num_extra_mux_bits) << 11),
>> +                                                 groups_total);
>> +
>> +       if (final_scale > 0x9) {
>> Why not just final_scale > 9?
>>
>
>We could use either, doesnt really matter in terms of compiler optimization. But
>from readibility pov I can change it to use 9 instead.

Yes, it just looked odd when I looked at it since other ifs do not have hex value while comparing.
>With that change can I consider your r-b?
>
The rest looks good.

Reviewed-by: Anusha Srivatsa <Anusha.srivatsa@xxxxxxxxx>

>Manasi
>
>> Anusha
>> +               /*
>> +                * ScaleIncrementInterval =
>> +                * finaloffset/((NflBpgOffset + SliceBpgOffset)*8(finalscale - 1.125))
>> +                * as (NflBpgOffset + SliceBpgOffset) has 11 bit fractional value,
>> +                * we need divide by 2^11 from pstDscCfg values
>> +                */
>> +               vdsc_cfg->scale_increment_interval =
>> +                               (vdsc_cfg->final_offset * (1 << 11)) /
>> +                               ((vdsc_cfg->nfl_bpg_offset +
>> +                               vdsc_cfg->slice_bpg_offset)*
>> +                               (final_scale - 9));
>> +       } else {
>> +               /*
>> +                * If finalScaleValue is less than or equal to 9, a value of 0 should
>> +                * be used to disable the scale increment at the end of the slice
>> +                */
>> +               vdsc_cfg->scale_increment_interval = 0;
>> +       }
>> +
>> +       if (vdsc_cfg->scale_increment_interval > 65535) {
>> +               DRM_ERROR("ScaleIncrementInterval is large for slice height\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /*
>> +        * DSC spec mentions that bits_per_pixel specifies the target
>> +        * bits/pixel (bpp) rate that is used by the encoder,
>> +        * in steps of 1/16 of a bit per pixel
>> +        */
>> +       rbs_min = vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset +
>> +               DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay *
>> +                            vdsc_cfg->bits_per_pixel, 16) +
>> +               groups_per_line * vdsc_cfg->first_line_bpg_offset;
>> +
>> +       hrd_delay = DIV_ROUND_UP((rbs_min * 16), vdsc_cfg->bits_per_pixel);
>> +       vdsc_cfg->rc_bits = (hrd_delay * vdsc_cfg->bits_per_pixel) / 16;
>> +       vdsc_cfg->initial_dec_delay = hrd_delay -
>> + vdsc_cfg->initial_xmit_delay;
>> +
>> +       return 0;
>> +}
>> +
>> +
>>  int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
>>                                 struct intel_crtc_state *pipe_config)
>> { @@ -451,5 +575,8 @@ int intel_dp_compute_dsc_params(struct intel_dp
>> *intel_dp,
>>         vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
>>                 (vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
>>
>> +       if (intel_compute_rc_parameters(vdsc_cfg) < 0)
>> +               return -1;
>> +
>>         return 0;
>>  }
>> --
>> 2.18.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux