Re: [Intel-gfx] [PATCH 1/8] drm/i915/dsc: improve clarify of the pps reg read/write helpers

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 07 Sep 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> Subject: [PATCH 1/8] drm/i915/dsc: improve clarify of the pps reg read/write
>> helpers
>
> Should be clarity here in the commit header

Thanks, fixed.

>
> With that fixed
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

Thanks for the reviews, pushed the lot to drm-intel-next.

BR,
Jani.


>>
>> Make it clear what's the number of vdsc per pipe, and what's the number of
>> registers to grab. Have intel_dsc_get_pps_reg() return the registers it knows
>> even if the requested amount is bigger.
>>
>> Cc: Suraj Kandpal <suraj.kandpal@intel.com>
>> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_vdsc.c | 40 ++++++++++++-----------
>>  1 file changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> index b24601d0b2c5..14317bb6d3df 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> @@ -372,7 +372,7 @@ int intel_dsc_get_num_vdsc_instances(const struct
>> intel_crtc_state *crtc_state)  }
>>
>>  static void intel_dsc_get_pps_reg(const struct intel_crtc_state *crtc_state, int
>> pps,
>> -                               i915_reg_t *dsc_reg, int vdsc_per_pipe)
>> +                               i915_reg_t *dsc_reg, int dsc_reg_num)
>>  {
>>       struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>       enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; @@ -
>> 381,16 +381,12 @@ static void intel_dsc_get_pps_reg(const struct
>> intel_crtc_state *crtc_state, int
>>
>>       pipe_dsc = is_pipe_dsc(crtc, cpu_transcoder);
>>
>> -     switch (vdsc_per_pipe) {
>> -     case 2:
>> +     if (dsc_reg_num >= 3)
>> +             MISSING_CASE(dsc_reg_num);
>> +     if (dsc_reg_num >= 2)
>>               dsc_reg[1] = pipe_dsc ? ICL_DSC1_PPS(pipe, pps) :
>> DSCC_PPS(pps);
>> -             fallthrough;
>> -     case 1:
>> +     if (dsc_reg_num >= 1)
>>               dsc_reg[0] = pipe_dsc ? ICL_DSC0_PPS(pipe, pps) :
>> DSCA_PPS(pps);
>> -             break;
>> -     default:
>> -             MISSING_CASE(vdsc_per_pipe);
>> -     }
>>  }
>>
>>  static void intel_dsc_write_pps_reg(const struct intel_crtc_state *crtc_state,
>> @@ -399,13 +395,16 @@ static void intel_dsc_write_pps_reg(const struct
>> intel_crtc_state *crtc_state,
>>       struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>       struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>>       i915_reg_t dsc_reg[2];
>> -     int i, vdsc_per_pipe = intel_dsc_get_vdsc_per_pipe(crtc_state);
>> +     int i, vdsc_per_pipe, dsc_reg_num;
>> +
>> +     vdsc_per_pipe = intel_dsc_get_vdsc_per_pipe(crtc_state);
>> +     dsc_reg_num = min_t(int, ARRAY_SIZE(dsc_reg), vdsc_per_pipe);
>>
>> -     drm_WARN_ON_ONCE(&i915->drm, ARRAY_SIZE(dsc_reg) <
>> vdsc_per_pipe);
>> +     drm_WARN_ON_ONCE(&i915->drm, dsc_reg_num < vdsc_per_pipe);
>>
>> -     intel_dsc_get_pps_reg(crtc_state, pps, dsc_reg, vdsc_per_pipe);
>> +     intel_dsc_get_pps_reg(crtc_state, pps, dsc_reg, dsc_reg_num);
>>
>> -     for (i = 0; i < min_t(int, ARRAY_SIZE(dsc_reg), vdsc_per_pipe); i++)
>> +     for (i = 0; i < dsc_reg_num; i++)
>>               intel_de_write(i915, dsc_reg[i], pps_val);  }
>>
>> @@ -815,16 +814,19 @@ static bool intel_dsc_read_pps_reg(struct
>> intel_crtc_state *crtc_state,  {
>>       struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>       struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>> -     const int vdsc_per_pipe = intel_dsc_get_vdsc_per_pipe(crtc_state);
>>       i915_reg_t dsc_reg[2];
>> -     int i;
>> +     int i, vdsc_per_pipe, dsc_reg_num;
>>
>> -     *pps_val = 0;
>> -     drm_WARN_ON_ONCE(&i915->drm, ARRAY_SIZE(dsc_reg) <
>> vdsc_per_pipe);
>> +     vdsc_per_pipe = intel_dsc_get_vdsc_per_pipe(crtc_state);
>> +     dsc_reg_num = min_t(int, ARRAY_SIZE(dsc_reg), vdsc_per_pipe);
>>
>> -     intel_dsc_get_pps_reg(crtc_state, pps, dsc_reg, vdsc_per_pipe);
>> +     drm_WARN_ON_ONCE(&i915->drm, dsc_reg_num < vdsc_per_pipe);
>> +
>> +     intel_dsc_get_pps_reg(crtc_state, pps, dsc_reg, dsc_reg_num);
>> +
>> +     *pps_val = 0;
>>
>> -     for (i = 0; i < min_t(int, ARRAY_SIZE(dsc_reg), vdsc_per_pipe); i++) {
>> +     for (i = 0; i < dsc_reg_num; i++) {
>>               u32 pps_temp;
>>
>>               pps_temp = intel_de_read(i915, dsc_reg[i]);
>> --
>> 2.39.2
>

-- 
Jani Nikula, Intel Open Source Graphics Center