Re: [Freedreno] [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 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@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?

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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux