[Freedreno] [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method

[Jessica Zhang]

On 6/15/2022 9:17 AM, Dmitry Baryshkov wrote:
> On 15/06/2022 19:11, Jessica Zhang wrote:
>> On 6/15/2022 2:35 AM, Dmitry Baryshkov wrote:
>>> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang 
>>> <quic_jesszhan at quicinc.com> wrote:
>>>>
>>>> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
>>>> helper method. This way, we can make it easier to get CRCs from 
>>>> other HW
>>>> blocks by adding other get_crc helper methods.
>>>>
>>>> Changes since V1:
>>>> - Moved common bitmasks to dpu_hw_util.h
>>>> - Moved common CRC methods to dpu_hw_util.c
>>>> - Updated copyrights
>>>> - Changed crcs array to a dynamically allocated array and added it as a
>>>>    member of crtc_state
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>
>>> Please split this into separate patches. One for hw_util, one for the 
>>> rest
>>
>> Sure
>>
>>>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 88 
>>>> +++++++++++++--------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  4 +
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c   | 42 +---------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
>>>>   5 files changed, 124 insertions(+), 72 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> index b56f777dbd0e..16742a66878e 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> @@ -1,5 +1,6 @@
>>>>   // SPDX-License-Identifier: GPL-2.0-only
>>>>   /*
>>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
>>>> reserved.
>>>>    * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>>>>    * Copyright (C) 2013 Red Hat
>>>>    * Author: Rob Clark <robdclark at gmail.com>
>>>> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct 
>>>> drm_crtc *crtc,
>>>>          enum dpu_crtc_crc_source source = 
>>>> dpu_crtc_parse_crc_source(src_name);
>>>>          struct dpu_crtc_state *crtc_state = 
>>>> to_dpu_crtc_state(crtc->state);
>>>>
>>>> +       if (crtc_state->crcs) {
>>>> +               kfree(crtc_state->crcs);
>>>> +               crtc_state->crcs = NULL;
>>>> +       }
>>>> +
>>>>          if (source < 0) {
>>>>                  DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n", 
>>>> src_name, crtc->index);
>>>>                  return -EINVAL;
>>>> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct 
>>>> drm_crtc *crtc,
>>>>          if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>>>>                  *values_cnt = crtc_state->num_mixers;
>>>>
>>>> +       crtc_state->crcs = kcalloc(*values_cnt, 
>>>> sizeof(crtc_state->crcs), GFP_KERNEL);
>>>> +
>>>
>>> I do not quite like the idea of constantly allocating and freeing the
>>> crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
>>> static array and verifying that no one over commits the
>>> MAX_CRC_VALUES.
>>> If at some point we decide that we really need the dynamic array, we
>>> can implement it later. We already had dynamic arrays and Rob had to
>>> fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
>>> atomic context").
>>
>> Ah, got it... the reason I used a dynamic array here was because AFAIK 
>> we don't have a defined upper limit for how many drm_encoders can be 
>> connected to a CRTC simultaneously. Do you have a suggestion on what 
>> value we can set for MAX_CRC_VALUES?
> 
> Three? Two for two hw_intfs?

Do you mean 2 hw_intfs per drm_encoder or 2 hw_intfs overall? IIRC we 
also wanted to take into account the possibility of there being multiple 
drm_encoders attached to a single CRTC.

Also, looking through Rob's fix, the warning was occuring because we 
were trying to call kcalloc in an IRQ context. However, the way I'm 
currently doing dynamic allocation will avoid the warning (since I'm 
doing kcalloc in verify_crc_source, which is called during the debugfs 
read/write/open and not during vblank). So I don't believe that we'll 
encounter the warning related to Rob's fix with my current 
implementation of the crcs dynamic array.

Thanks,

Jessica Zhang

> 
> 
> -- 
> With best wishes
> Dmitry