[PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
Jessica Zhang
quic_jesszhan at quicinc.com
Wed Jun 15 16:11:46 UTC 2022
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?
>
>> return 0;
>> }
>>
>> +static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
>> +{
>> + struct dpu_crtc_mixer *m;
>> + int i;
>> +
>> + for (i = 0; i < crtc_state->num_mixers; ++i) {
>> + m = &crtc_state->mixers[i];
>> +
>> + if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
>> + continue;
>> +
>> + /* Calculate MISR over 1 frame */
>> + m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
>> + }
>> +}
>> +
>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>> {
>> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
>> enum dpu_crtc_crc_source current_source;
>> struct dpu_crtc_state *crtc_state;
>> struct drm_device *drm_dev = crtc->dev;
>> - struct dpu_crtc_mixer *m;
>>
>> bool was_enabled;
>> bool enable = false;
>> - int i, ret = 0;
>> + int ret = 0;
>>
>> if (source < 0) {
>> DRM_DEBUG_DRIVER("Invalid CRC source %s for CRTC%d\n", src_name, crtc->index);
>> @@ -137,6 +160,10 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>> goto cleanup;
>>
>> } else if (was_enabled && !enable) {
>> + if (crtc_state->crcs) {
>> + kfree(crtc_state->crcs);
>> + crtc_state->crcs = NULL;
>> + }
>> drm_crtc_vblank_put(crtc);
>> }
>>
>> @@ -146,16 +173,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>>
>> crtc_state->crc_frame_skip_count = 0;
>>
>> - for (i = 0; i < crtc_state->num_mixers; ++i) {
>> - m = &crtc_state->mixers[i];
>> -
>> - if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
>> - continue;
>> -
>> - /* Calculate MISR over 1 frame */
>> - m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
>> - }
>> -
>> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> + dpu_crtc_setup_lm_misr(crtc_state);
>
> else ?
Noted.
>
>>
>> cleanup:
>> drm_modeset_unlock(&crtc->mutex);
>> @@ -174,34 +193,21 @@ static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
>> return dpu_encoder_get_vsync_count(encoder);
>> }
>>
>> -
>> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crtc_state)
>> {
>> - struct dpu_crtc_state *crtc_state;
>> - struct dpu_crtc_mixer *m;
>> - u32 crcs[CRTC_DUAL_MIXERS];
>> + struct dpu_crtc_mixer *lm;
>>
>> - int i = 0;
>> int rc = 0;
>> -
>> - crtc_state = to_dpu_crtc_state(crtc->state);
>> -
>> - BUILD_BUG_ON(ARRAY_SIZE(crcs) != ARRAY_SIZE(crtc_state->mixers));
>> -
>> - /* Skip first 2 frames in case of "uncooked" CRCs */
>> - if (crtc_state->crc_frame_skip_count < 2) {
>> - crtc_state->crc_frame_skip_count++;
>> - return 0;
>> - }
>> + int i;
>>
>> for (i = 0; i < crtc_state->num_mixers; ++i) {
>>
>> - m = &crtc_state->mixers[i];
>> + lm = &crtc_state->mixers[i];
>>
>> - if (!m->hw_lm || !m->hw_lm->ops.collect_misr)
>> + if (!lm->hw_lm || !lm->hw_lm->ops.collect_misr)
>> continue;
>>
>> - rc = m->hw_lm->ops.collect_misr(m->hw_lm, &crcs[i]);
>> + rc = lm->hw_lm->ops.collect_misr(lm->hw_lm, &crtc_state->crcs[i]);
>>
>> if (rc) {
>> if (rc != -ENODATA)
>> @@ -211,7 +217,25 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> }
>>
>> return drm_crtc_add_crc_entry(crtc, true,
>> - drm_crtc_accurate_vblank_count(crtc), crcs);
>> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
>> +}
>> +
>> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +{
>> + struct dpu_crtc_state *crtc_state;
>> +
>> + crtc_state = to_dpu_crtc_state(crtc->state);
>> +
>> + /* Skip first 2 frames in case of "uncooked" CRCs */
>> + if (crtc_state->crc_frame_skip_count < 2) {
>> + crtc_state->crc_frame_skip_count++;
>> + return 0;
>> + }
>> +
>> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> + return dpu_crtc_get_lm_crc(crtc, crtc_state);
>> +
>> + return 0;
>> }
>>
>> static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index b8785c394fcc..4bf45e3343ef 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -201,6 +201,9 @@ struct dpu_crtc {
>> * @mixers : List of active mixers
>> * @num_ctls : Number of ctl paths in use
>> * @hw_ctls : List of active ctl paths
>> + * @crc_source : CRC source
>> + * @crc_frame_skip_count: Number of frames skipped before getting CRC
>> + * @crcs : Array to store CRC values
>> */
>> struct dpu_crtc_state {
>> struct drm_crtc_state base;
>> @@ -222,6 +225,7 @@ struct dpu_crtc_state {
>>
>> enum dpu_crtc_crc_source crc_source;
>> int crc_frame_skip_count;
>> + u32 *crcs;
>> };
>>
>> #define to_dpu_crtc_state(x) \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> index 462f5082099e..06b24d8d1419 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> */
>>
>> @@ -27,11 +28,6 @@
>>
>> #define LM_MISR_CTRL 0x310
>> #define LM_MISR_SIGNATURE 0x314
>> -#define LM_MISR_FRAME_COUNT_MASK 0xFF
>> -#define LM_MISR_CTRL_ENABLE BIT(8)
>> -#define LM_MISR_CTRL_STATUS BIT(9)
>> -#define LM_MISR_CTRL_STATUS_CLEAR BIT(10)
>> -#define LM_MISR_CTRL_FREE_RUN_MASK BIT(31)
>>
>>
>> static const struct dpu_lm_cfg *_lm_offset(enum dpu_lm mixer,
>> @@ -108,44 +104,12 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx,
>>
>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count)
>> {
>> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
>> - u32 config = 0;
>> -
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, LM_MISR_CTRL_STATUS_CLEAR);
>> -
>> - /* Clear old MISR value (in case it's read before a new value is calculated)*/
>> - wmb();
>> -
>> - if (enable) {
>> - config = (frame_count & LM_MISR_FRAME_COUNT_MASK) |
>> - LM_MISR_CTRL_ENABLE | LM_MISR_CTRL_FREE_RUN_MASK;
>> -
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, config);
>> - } else {
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, 0);
>> - }
>> -
>> + dpu_hw_setup_misr(&ctx->hw, enable, frame_count, LM_MISR_CTRL);
>> }
>>
>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value)
>> {
>> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
>> - u32 ctrl = 0;
>> -
>> - if (!misr_value)
>> - return -EINVAL;
>> -
>> - ctrl = DPU_REG_READ(c, LM_MISR_CTRL);
>> -
>> - if (!(ctrl & LM_MISR_CTRL_ENABLE))
>> - return -ENODATA;
>> -
>> - if (!(ctrl & LM_MISR_CTRL_STATUS))
>> - return -EINVAL;
>> -
>> - *misr_value = DPU_REG_READ(c, LM_MISR_SIGNATURE);
>> -
>> - return 0;
>> + return dpu_hw_collect_misr(&ctx->hw, misr_value, LM_MISR_CTRL, LM_MISR_SIGNATURE);
>> }
>>
>> static void dpu_hw_lm_setup_blend_config_sdm845(struct dpu_hw_mixer *ctx,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> index 512316f25a51..244f2f90e99a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> @@ -1,5 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> */
>> #define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
>>
>> @@ -447,3 +449,45 @@ u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>>
>> return 0;
>> }
>> +
>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, bool enable,
>> + u32 frame_count, u32 misr_ctrl_offset)
>> +{
>> + u32 config = 0;
>> +
>> + DPU_REG_WRITE(c, misr_ctrl_offset, MISR_CTRL_STATUS_CLEAR);
>> +
>> + /* Clear old MISR value (in case it's read before a new value is calculated)*/
>> + wmb();
>> +
>> + if (enable) {
>> + config = (frame_count & MISR_FRAME_COUNT_MASK) |
>> + MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK;
>> +
>> + DPU_REG_WRITE(c, misr_ctrl_offset, config);
>> + } else {
>> + DPU_REG_WRITE(c, misr_ctrl_offset, 0);
>> + }
>> +
>> +}
>> +
>> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 *misr_value,
>> + u32 misr_ctrl_offset, u32 misr_signature_offset)
>> +{
>> + u32 ctrl = 0;
>> +
>> + if (!misr_value)
>> + return -EINVAL;
>> +
>> + ctrl = DPU_REG_READ(c, misr_ctrl_offset);
>> +
>> + if (!(ctrl & MISR_CTRL_ENABLE))
>> + return -ENODATA;
>> +
>> + if (!(ctrl & MISR_CTRL_STATUS))
>> + return -EINVAL;
>> +
>> + *misr_value = DPU_REG_READ(c, misr_signature_offset);
>> +
>> + return 0;
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> index e4a65eb4f769..88df3a5c5d8e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> @@ -1,5 +1,6 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> */
>>
>> @@ -12,6 +13,11 @@
>> #include "dpu_hw_catalog.h"
>>
>> #define REG_MASK(n) ((BIT(n)) - 1)
>> +#define MISR_FRAME_COUNT_MASK 0xFF
>> +#define MISR_CTRL_ENABLE BIT(8)
>> +#define MISR_CTRL_STATUS BIT(9)
>> +#define MISR_CTRL_STATUS_CLEAR BIT(10)
>> +#define MISR_CTRL_FREE_RUN_MASK BIT(31)
>>
>> /*
>> * This is the common struct maintained by each sub block
>> @@ -343,4 +349,14 @@ void dpu_hw_csc_setup(struct dpu_hw_blk_reg_map *c,
>> u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>> u32 total_fl);
>>
>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
>> + bool enable,
>> + u32 frame_count,
>> + u32 misr_ctrl_offset);
>> +
>> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c,
>> + u32 *misr_value,
>> + u32 misr_ctrl_offset,
>> + u32 misr_signature_offset);
>
> Please move offsets next to the hw_blk_reg_map.
Noted.
Thanks,
Jessica Zhang
>
>> +
>> #endif /* _DPU_HW_UTIL_H */
>> --
>> 2.35.1
>>
>
>
> --
> With best wishes
> Dmitry
More information about the dri-devel
mailing list