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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx> 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@xxxxxxxxxxx>

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@xxxxxxxxx>
@@ -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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux