On Tue, Nov 06, 2018 at 12:14:50PM -0800, Manasi Navare wrote:
> On Tue, Nov 06, 2018 at 07:00:50PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 06, 2018 at 08:52:41AM -0800, Manasi Navare wrote:
> > > On Tue, Nov 06, 2018 at 04:33:38PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 02, 2018 at 02:31:29PM -0700, Manasi Navare wrote:
> > > > > From: Gaurav K Singh <gaurav.k.si...@intel.com>
> > > > >
> > > > > 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.
> > > > >
> > > > > v7 (From Manasi):
> > > > > * Use DRM_DEBUG instead of DRM_ERROR (Ville)
> > > > > * Use Error numberinstead of -1 (Ville)
> > > > > v6 (From Manasi):
> > > > > * Use 9 instead of 0x9 for consistency (Anusha)
> > > > >
> > > > > 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.nav...@intel.com>
> > > > > Cc: Jani Nikula <jani.nik...@linux.intel.com>
> > > > > Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
> > > > > Signed-off-by: Gaurav K Singh <gaurav.k.si...@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com>
> > > > > Reviewed-by: Anusha Srivatsa <anusha.sriva...@intel.com>
> > > > > ---
> > > > > 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 0a1918f2f643..a76f78b9c0ee 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_DEBUG_KMS("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_DEBUG_KMS("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 > 9) {
> > > > > + /*
> > > > > + * 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_DEBUG_KMS("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 -EINVAL;
> > > >
> > > > One should pretty much always pass on the errno rather than inventing a
> > > > new one on the spot.
> > >
> > > But compute_rc_parameters() function returns EINVAL everywhere for error
> > > cases
> >
> > Why should I have to know that?
>
> After doing errno --list and looking at the available errnos, the most
> logical ones to return
> from intel_compute_rc_parameters is ERANGE since the errors are because the
> parameters of out of valid range.
I guess it doesn't really matter which one we pick here since
it won't go all the way out to userspace anyway.
> And here I can just do return intel_compute_rc_parameters()
Yes, that seems good.
>
> Does this look good?
>
> Manasi
>
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
