| Message ID | 20220614211328.216-2-quic_jesszhan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Expand CRC to support interface blocks | expand |
On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@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@quicinc.com> Please split this into separate patches. One for hw_util, one for the rest > --- > 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@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"). > 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 ? > > 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. > + > #endif /* _DPU_HW_UTIL_H */ > -- > 2.35.1 >
On 6/15/2022 2:35 AM, Dmitry Baryshkov wrote: > On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@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@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@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
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@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@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@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?
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@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@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@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? 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
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@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); + 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); 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); + #endif /* _DPU_HW_UTIL_H */
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@quicinc.com> --- 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(-)