[PATCH] drm/msm/dpu: Fix encoder CRC to account for CTM enablement
Abhinav Kumar
quic_abhinavk at quicinc.com
Wed Nov 22 00:59:30 UTC 2023
On 11/21/2023 4:27 PM, Rob Clark wrote:
> On Tue, Nov 21, 2023 at 4:41 PM Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 10/24/2023 12:01 PM, Abhinav Kumar wrote:
>>>
>>>
>>> On 10/23/2023 4:03 PM, Dmitry Baryshkov wrote:
>>>> On Tue, 24 Oct 2023 at 01:36, Rob Clark <robdclark at gmail.com> wrote:
>>>>>
>>>>> On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov
>>>>> <dmitry.baryshkov at linaro.org> wrote:
>>>>>>
>>>>>> On Tue, 24 Oct 2023 at 01:12, Rob Clark <robdclark at gmail.com> wrote:
>>>>>>>
>>>>>>> From: Rob Clark <robdclark at chromium.org>
>>>>>>>
>>>>>>> Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not
>>>>>>> otherwise.
>>>>>>>
>>>>>>> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>>> Signed-off-by: Rob Clark <robdclark at chromium.org>
>>>>>>> ---
>>>
>>> I cannot find anything in the docs which suggest this solution is correct.
>>>
>>> Different blocks in the DPU pipeline have their own CRC (MISR) registers
>>> like LM, intf etc.
>>>
>>> We dont need to change INPUT_SEL to tell DPU from which pipeline to take
>>> the CRC from as each of them have their own registers.
>>>
>>> INPUT_SEL is controlling whether the CRC needs to be calculated over the
>>> entire display timings or only the active pixels. I am unable to tell at
>>> the moment why this is making a difference in this use-case.
>>>
>>> Since I am unable to find any documentation proving this solution is
>>> correct so far, unfortunately I would hold this back till then.
>>>
>>> We will investigate this issue and report our findings on this thread on
>>> how to proceed.
>>>
>>
>> Alright, we debugged and also found some more answers.
>>
>> The correct solution is indeed to set INPUT_SEL = 1 but let me explain
>> why and what should be the correct way.
>>
>> INPUT_SEL was indeed telling whether to compute CRC over active pixels
>> or active pixels + timings like I wrote before but this behavior changed
>> since some chipsets.
>>
>> Now, INPUT_SEL = 0 means compute CRC *only* over timings and not the
>> active area (and not display + timings like before) and like mentioned
>> before this has nothing to do with what is the input to the CRC. Not
>> covering the active area will not change the CRC at all as Rob reported
>> but its not specific to CTM.
>>
>> Which means we should have been setting INPUT_SEL=1 whenever we use INTF
>> CRC irrespective of whether CTM is used or not.
>>
>> What this also means is INTF CRC was not working correctly at all so far
>> irrespecive of CTM or not because it was always computing CRC only on
>> the timings (non-active area).
>>
>> This was not caught so far because it looks like IGT's
>> kms_pipe_crc_basic test which was used to validate this only compares
>> CRC between two frames of the same content to match if they were equal
>> and not changing contents and comparing like kms_plane does. It will
>> pass as CRC would not have changed.
>>
>> Now coming to the fix, the reset value of this register INTF_MISR_CTRL
>> already sets the INPUT_SEL bit (or unsets it) correctly based on
>> whichever DPU version is used so we should just change the
>> dpu_hw_setup_misr() to a read on the register followed by ORing the
>> required bits without touching INPUT_SEL and write.
>>
>> That will address this issue and also cover version control since the
>> expected value of this bit has changed across DPU revisions.
>
> Ok, thanks for following up on this. Mind posting a patch to
> supersede this one?
>
> BR,
> -R
>
Yup, we will.
Thanks
Abhinav
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++--
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++-
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++--
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +-
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 ++++-
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++-
>>>>>>> 8 files changed, 15 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> index 2b83a13b3aa9..d93a92ffd5df 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct
>>>>>>> drm_crtc *crtc)
>>>>>>> struct drm_encoder *drm_enc;
>>>>>>>
>>>>>>> drm_for_each_encoder_mask(drm_enc, crtc->dev,
>>>>>>> crtc->state->encoder_mask)
>>>>>>> - dpu_encoder_setup_misr(drm_enc);
>>>>>>> + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm);
>>>>>>> }
>>>>>>>
>>>>>>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const
>>>>>>> char *src_name)
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>>> index b0a7908418ed..12ee7acb5ea6 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>>> @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct
>>>>>>> drm_encoder *drm_enc)
>>>>>>> return num_intf;
>>>>>>> }
>>>>>>>
>>>>>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
>>>>>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc,
>>>>>>> bool has_ctm)
>>>>>>> {
>>>>>>> struct dpu_encoder_virt *dpu_enc;
>>>>>>>
>>>>>>> @@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct
>>>>>>> drm_encoder *drm_enc)
>>>>>>> if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
>>>>>>> continue;
>>>>>>>
>>>>>>> - phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);
>>>>>>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true,
>>>>>>> 1, has_ctm);
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>>>>> index 4c05fd5e9ed1..510783b2fb24 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>>>>> @@ -169,8 +169,9 @@ int dpu_encoder_get_crc_values_cnt(const struct
>>>>>>> drm_encoder *drm_enc);
>>>>>>> /**
>>>>>>> * dpu_encoder_setup_misr - enable misr calculations
>>>>>>> * @drm_enc: Pointer to previously created drm encoder structure
>>>>>>> + * @has_ctm: Is CTM enabled
>>>>>>> */
>>>>>>> -void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder);
>>>>>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder,
>>>>>>> bool has_ctm);
>>>>>>>
>>>>>>> /**
>>>>>>> * dpu_encoder_get_crc - get the crc value from interface blocks
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>>> index e8b8908d3e12..cb06f80cc671 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>>> @@ -318,9 +318,9 @@ static u32 dpu_hw_intf_get_line_count(struct
>>>>>>> dpu_hw_intf *intf)
>>>>>>> return DPU_REG_READ(c, INTF_LINE_COUNT);
>>>>>>> }
>>>>>>>
>>>>>>> -static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool
>>>>>>> enable, u32 frame_count)
>>>>>>> +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool
>>>>>>> enable, u32 frame_count, bool has_ctm)
>>>>>>> {
>>>>>>> - dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable,
>>>>>>> frame_count);
>>>>>>> + dpu_hw_setup_misr(&intf->hw, INTF_MISR_CTRL, enable,
>>>>>>> frame_count, has_ctm);
>>>>>>
>>>>>> I'm not sure about the dpu_encoder and dpu_hw_intf interfaces. But
>>>>>> dpu_hw_setup_misr definitely needs the `u8 input_sel` parameter
>>>>>> instead of `bool has_ctm`.
>>>>>
>>>>> That seems a bit premature without knowing what the other values are.
>>>>> (And I also question a bit the whole abstraction layer thing if it is
>>>>> taking directly register bitfield enum's..)
>>>>
>>>> dpu_hw_intf and especially dpu_hw_util are not real abstractions. I
>>>> always viewed them as useful low-level helpers.
>>>>
>>>> I think that has_ctm is valid at the dpu_encoder level, which selects
>>>> which input to use. on the lower levels has_ctm doesn't make sense.
>>>> IOW dpu_hw_setup_misr can be used to setup MISR for other blocks,
>>>> where CTM doesn't exist.
>>>>
>>>>>
>>>>> BR,
>>>>> -R
>>>>>
>>>>>> Most likely, I'd use u8 for dpu_hw_intf operation too.
>>>>>>
>>>>>> Could you please adjust?
>>>>>>
>>>>>>> }
>>>>>>>
>>>>>>> static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32
>>>>>>> *misr_value)
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>>>>> index c539025c418b..95aafc4cf58e 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>>>>> @@ -95,7 +95,7 @@ struct dpu_hw_intf_ops {
>>>>>>>
>>>>>>> void (*bind_pingpong_blk)(struct dpu_hw_intf *intf,
>>>>>>> const enum dpu_pingpong pp);
>>>>>>> - void (*setup_misr)(struct dpu_hw_intf *intf, bool enable,
>>>>>>> u32 frame_count);
>>>>>>> + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable,
>>>>>>> u32 frame_count, bool has_ctm);
>>>>>>> int (*collect_misr)(struct dpu_hw_intf *intf, u32
>>>>>>> *misr_value);
>>>>>>>
>>>>>>> // Tearcheck on INTF since DPU 5.0.0
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>>>>>>> index d1c3bd8379ea..2efe29396c6a 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>>>>>>> @@ -83,7 +83,7 @@ static void dpu_hw_lm_setup_border_color(struct
>>>>>>> dpu_hw_mixer *ctx,
>>>>>>>
>>>>>>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool
>>>>>>> enable, u32 frame_count)
>>>>>>> {
>>>>>>> - dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable,
>>>>>>> frame_count);
>>>>>>> + dpu_hw_setup_misr(&ctx->hw, LM_MISR_CTRL, enable,
>>>>>>> frame_count, false);
>>>>>>> }
>>>>>>>
>>>>>>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32
>>>>>>> *misr_value)
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>>>>>>> index 9d2273fd2fed..528b8439209f 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>>>>>>> @@ -483,7 +483,7 @@ void _dpu_hw_setup_qos_lut(struct
>>>>>>> dpu_hw_blk_reg_map *c, u32 offset,
>>>>>>>
>>>>>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
>>>>>>> u32 misr_ctrl_offset,
>>>>>>> - bool enable, u32 frame_count)
>>>>>>> + bool enable, u32 frame_count, bool has_ctm)
>>>>>>> {
>>>>>>> u32 config = 0;
>>>>>>>
>>>>>>> @@ -496,6 +496,9 @@ void dpu_hw_setup_misr(struct
>>>>>>> dpu_hw_blk_reg_map *c,
>>>>>>> config = (frame_count & MISR_FRAME_COUNT_MASK) |
>>>>>>> MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK;
>>>>>>>
>>>>>>> + if (!has_ctm)
>>>>>>> + config |= 1 << 24;
>>>>>>
>>>>>> Please define MISR_CTRL_INPUT_SEL instead.
>>>>>>
>>>>>>> +
>>>>>>> DPU_REG_WRITE(c, misr_ctrl_offset, config);
>>>>>>> } else {
>>>>>>> DPU_REG_WRITE(c, misr_ctrl_offset, 0);
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>>>>>>> index 1f6079f47071..e42d9d00e40e 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>>>>>>> @@ -360,7 +360,8 @@ void _dpu_hw_setup_qos_lut(struct
>>>>>>> dpu_hw_blk_reg_map *c, u32 offset,
>>>>>>> void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
>>>>>>> u32 misr_ctrl_offset,
>>>>>>> bool enable,
>>>>>>> - u32 frame_count);
>>>>>>> + u32 frame_count,
>>>>>>> + bool has_ctm);
>>>>>>>
>>>>>>> int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c,
>>>>>>> u32 misr_ctrl_offset,
>>>>>>> --
>>>>>>> 2.41.0
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> With best wishes
>>>>>> Dmitry
>>>>
>>>>
>>>>
More information about the dri-devel
mailing list