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

[Dmitry Baryshkov]

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?


-- 
With best wishes
Dmitry