Re: [PATCH 04/10] drm/i915/vdsc: Add support for read/write PPS for DSC3

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

Hi Suraj,

Thanks for the review and comments.

I agree with most of the comments on the series and will send new 
version with those addressed.

Please find my explanation inline for your query:

On 10/18/2024 7:47 AM, Kandpal, Suraj wrote:
>
>> -----Original Message-----
>> From: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>
>> Sent: Thursday, October 17, 2024 1:54 PM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: intel-xe@lists.freedesktop.org; Kandpal, Suraj
>> <suraj.kandpal@intel.com>
>> Subject: [PATCH 04/10] drm/i915/vdsc: Add support for read/write PPS for
>> DSC3
>>
>> With BMG each pipe has 3 DSC engines, so add bits to read/write the PPS
>> registers for the 3rd VDSC engine.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vdsc.c      | 8 +++++---
>>   drivers/gpu/drm/i915/display/intel_vdsc_regs.h | 6 ++++++
>>   2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> index e34483d5be36..718e1b400af5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> @@ -411,8 +411,10 @@ static void intel_dsc_get_pps_reg(const struct
>> intel_crtc_state *crtc_state, int
>>
>>   	pipe_dsc = is_pipe_dsc(crtc, cpu_transcoder);
>>
>> -	if (dsc_reg_num >= 3)
>> +	if (dsc_reg_num >= 4)
>>   		MISSING_CASE(dsc_reg_num);
>> +	if (dsc_reg_num >= 3)
>> +		dsc_reg[2] = BMG_DSC2_PPS(pipe, pps);
>>   	if (dsc_reg_num >= 2)
> Quick question why is this condition not == that would make sense only the first condition
> Should have been >=

We want to set dsc_reg[1] for case where dsc_reg_num is 2 as well as 
when its 3 as we need to write PPS for 3 DSC engines.

Changing to 'if (dsc_reg_num == 2)' will only set dsc_reg[2], but skip 
dsc_reg[1] when dsc_reg_num is 3.

Thanks & Regards,

Ankit


> Maybe another patch to fix this
> Rest LGTM,
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
>
>>   		dsc_reg[1] = pipe_dsc ? ICL_DSC1_PPS(pipe, pps) :
>> DSCC_PPS(pps);
>>   	if (dsc_reg_num >= 1)
>> @@ -424,7 +426,7 @@ static void intel_dsc_pps_write(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];
>> +	i915_reg_t dsc_reg[3];
>>   	int i, vdsc_per_pipe, dsc_reg_num;
>>
>>   	vdsc_per_pipe = intel_dsc_get_vdsc_per_pipe(crtc_state);
>> @@ -824,7 +826,7 @@ static u32 intel_dsc_pps_read(struct intel_crtc_state
>> *crtc_state, int pps,  {
>>   	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];
>> +	i915_reg_t dsc_reg[3];
>>   	int i, vdsc_per_pipe, dsc_reg_num;
>>   	u32 val;
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
>> b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
>> index 941f4ff6b940..efaeb5e0aea3 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
>> @@ -61,8 +61,10 @@
>>   #define DSCC_PPS(pps)				_MMIO(_DSCC_PPS_0
>> + ((pps) < 12 ? (pps) : (pps) + 12) * 4)
>>   #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB	0x78270
>>   #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB	0x78370
>> +#define _BMG_DSC2_PICTURE_PARAMETER_SET_0_PB	0x78970
>>   #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC	0x78470
>>   #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC	0x78570
>> +#define _BMG_DSC2_PICTURE_PARAMETER_SET_0_PC	0x78A70
>>   #define ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe)	_MMIO_PIPE((pipe) -
>> PIPE_B, \
>>
>> _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB, \
>>
>> _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC)
>> @@ -75,8 +77,12 @@
>>   #define _ICL_DSC1_PPS_0(pipe)			_PICK_EVEN((pipe) -
>> PIPE_B, \
>>
>> _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB, \
>>
>> _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC)
>> +#define _BMG_DSC2_PPS_0(pipe)			_PICK_EVEN((pipe) -
>> PIPE_B, \
>> +
>> _BMG_DSC2_PICTURE_PARAMETER_SET_0_PB, \
>> +
>> _BMG_DSC2_PICTURE_PARAMETER_SET_0_PC)
>>   #define  ICL_DSC0_PPS(pipe, pps)
>> 	_MMIO(_ICL_DSC0_PPS_0(pipe) + ((pps) * 4))
>>   #define  ICL_DSC1_PPS(pipe, pps)
>> 	_MMIO(_ICL_DSC1_PPS_0(pipe) + ((pps) * 4))
>> +#define  BMG_DSC2_PPS(pipe, pps)
>> 	_MMIO(_BMG_DSC2_PPS_0(pipe) + ((pps) * 4))
>>
>>   /* PPS 0 */
>>   #define   DSC_PPS0_NATIVE_422_ENABLE		REG_BIT(23)
>> --
>> 2.45.2