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
