| Message ID | 20220527185407.162-4-quic_jesszhan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Expand CRC to support interface blocks | expand |
On 27/05/2022 21:54, Jessica Zhang wrote: > Add support for writing CRC values for the interface block to > the debugfs by calling the necessary MISR setup/collect methods. > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ > 4 files changed, 128 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index ae09466663cf..e830fb1e910d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name) > if (!strcmp(src_name, "auto") || > !strcmp(src_name, "lm")) > return DPU_CRTC_CRC_SOURCE_LAYER_MIXER; > + if (!strcmp(src_name, "intf")) > + return DPU_CRTC_CRC_SOURCE_INTF; > > return DPU_CRTC_CRC_SOURCE_INVALID; > } > @@ -94,8 +96,18 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc, > return -EINVAL; > } > > - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) > + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) { > *values_cnt = crtc_state->num_mixers; > + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) { > + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); Let's do this correctly from the beginning. The CRTC can drive several encoders. Do we want to get CRC from all of them or just from the first one? > + > + if (!drm_enc) { > + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); > + return -ENODATA; > + } > + > + *values_cnt = dpu_encoder_get_num_phys(drm_enc); > + } > > return 0; > } > @@ -116,6 +128,18 @@ static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state) > } > } > > +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) > +{ > + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); > + > + if (!drm_enc) { > + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); > + return; > + } > + > + dpu_encoder_setup_misr(drm_enc); > +} > + > 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); > @@ -164,6 +188,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name) > > if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) > dpu_crtc_setup_lm_misr(crtc_state); > + else if (source == DPU_CRTC_CRC_SOURCE_INTF) > + dpu_crtc_setup_encoder_misr(crtc); > > cleanup: > drm_modeset_unlock(&crtc->mutex); > @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt > drm_crtc_accurate_vblank_count(crtc), crcs); > } > > +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) > +{ > + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); > + > + if (!drm_enc) { > + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); > + return -EINVAL; > + } > + > + return dpu_encoder_get_crc(drm_enc); > +} > + > static int dpu_crtc_get_crc(struct drm_crtc *crtc) > { > struct dpu_crtc_state *crtc_state; > @@ -227,6 +265,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc) > if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) > return dpu_crtc_get_lm_crc(crtc, crtc_state); > > + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF) > + return dpu_crtc_get_encoder_crc(crtc); > + > return 0; > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > index b8785c394fcc..a60af034905d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.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. > * Copyright (C) 2013 Red Hat > * Author: Rob Clark <robdclark@gmail.com> > @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data { > * enum dpu_crtc_crc_source: CRC source > * @DPU_CRTC_CRC_SOURCE_NONE: no source set > * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer > + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface > * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source > */ > enum dpu_crtc_crc_source { > DPU_CRTC_CRC_SOURCE_NONE = 0, > DPU_CRTC_CRC_SOURCE_LAYER_MIXER, > + DPU_CRTC_CRC_SOURCE_INTF, > DPU_CRTC_CRC_SOURCE_MAX, > DPU_CRTC_CRC_SOURCE_INVALID = -1 > }; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 52516eb20cb8..7740515f462d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -14,6 +14,7 @@ > > #include <drm/drm_crtc.h> > #include <drm/drm_file.h> > +#include <drm/drm_vblank.h> > #include <drm/drm_probe_helper.h> > > #include "msm_drv.h" > @@ -225,6 +226,66 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) > return dpu_enc->wide_bus_en; > } > > +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc) > +{ > + struct dpu_encoder_virt *dpu_enc; > + > + dpu_enc = to_dpu_encoder_virt(drm_enc); > + > + return dpu_enc->num_phys_encs; > +} > + > +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) > +{ > + struct dpu_encoder_virt *dpu_enc; > + > + int i; > + > + dpu_enc = to_dpu_encoder_virt(drm_enc); > + > + for (i = 0; i < dpu_enc->num_phys_encs; i++) { > + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > + > + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) > + continue; Does WB support CRC? > + > + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); > + } > +} > + > +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) > +{ > + struct dpu_encoder_virt *dpu_enc; > + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; > + > + int i, rc; > + > + if (!drm_enc->crtc) { > + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index); > + return -EINVAL; > + } > + > + dpu_enc = to_dpu_encoder_virt(drm_enc); > + > + for (i = 0; i < dpu_enc->num_phys_encs; i++) { > + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > + > + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr) > + continue; > + > + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[i]); This doesn't look fully correct. Do we need to skip the indices for the phys without a backing hw_intf? > + Extra empty line. > + if (rc) { > + if (rc != -ENODATA) Do we need to handle ENODATA in any specific way (like zeroing the crcs[i])? If not, I'd suggest to drop this return code. Let's make an error always an error. > + DRM_DEBUG_DRIVER("MISR read failed\n"); > + return rc; > + } > + } > + > + return drm_crtc_add_crc_entry(drm_enc->crtc, true, > + drm_crtc_accurate_vblank_count(drm_enc->crtc), crcs); > +} > + > static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc) > { > struct dpu_hw_dither_cfg dither_cfg = { 0 }; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > index 781d41c91994..8345599dd01a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.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-2018, The Linux Foundation. All rights reserved. > * Copyright (C) 2013 Red Hat > * Author: Rob Clark <robdclark@gmail.com> > @@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc); > > bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); > > +/** > + * dpu_encoder_get_num_phys - get number of physical encoders contained in virtual > + * encoder > + * @drm_enc: Pointer to previously created drm encoder structure > + * Returns: Number of physical encoders for given drm encoder > + */ > +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc); > + > +/** > + * dpu_encoder_setup_misr - enable misr calculations > + * @drm_enc: Pointer to previously created drm encoder structure > + */ > +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); > + > +/** > + * dpu_encoder_get_crc - get the crc value from interface blocks > + * @drm_enc: Pointer to previously created drm encoder structure > + * Returns: 0 on success, error otherwise > + */ > +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc); > + > /** > * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology. > * @drm_enc: Pointer to previously created drm encoder structure
On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote: > On 27/05/2022 21:54, Jessica Zhang wrote: >> Add support for writing CRC values for the interface block to >> the debugfs by calling the necessary MISR setup/collect methods. >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ >> 4 files changed, 128 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> index ae09466663cf..e830fb1e910d 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source >> dpu_crtc_parse_crc_source(const char *src_name) >> if (!strcmp(src_name, "auto") || >> !strcmp(src_name, "lm")) >> return DPU_CRTC_CRC_SOURCE_LAYER_MIXER; >> + if (!strcmp(src_name, "intf")) >> + return DPU_CRTC_CRC_SOURCE_INTF; >> return DPU_CRTC_CRC_SOURCE_INVALID; >> } >> @@ -94,8 +96,18 @@ static int dpu_crtc_verify_crc_source(struct >> drm_crtc *crtc, >> return -EINVAL; >> } >> - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) { >> *values_cnt = crtc_state->num_mixers; >> + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) { >> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); > > Let's do this correctly from the beginning. The CRTC can drive several > encoders. Do we want to get CRC from all of them or just from the first > one? In the case of multiple encoders, it would be better to collect CRCs from all of them. > >> + >> + if (!drm_enc) { >> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >> + return -ENODATA; >> + } >> + >> + *values_cnt = dpu_encoder_get_num_phys(drm_enc); >> + } >> return 0; >> } >> @@ -116,6 +128,18 @@ static void dpu_crtc_setup_lm_misr(struct >> dpu_crtc_state *crtc_state) >> } >> } >> +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) >> +{ >> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); >> + >> + if (!drm_enc) { >> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >> + return; >> + } >> + >> + dpu_encoder_setup_misr(drm_enc); >> +} >> + >> 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); >> @@ -164,6 +188,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc >> *crtc, const char *src_name) >> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >> dpu_crtc_setup_lm_misr(crtc_state); >> + else if (source == DPU_CRTC_CRC_SOURCE_INTF) >> + dpu_crtc_setup_encoder_misr(crtc); >> cleanup: >> drm_modeset_unlock(&crtc->mutex); >> @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc >> *crtc, struct dpu_crtc_state *crt >> drm_crtc_accurate_vblank_count(crtc), crcs); >> } >> +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) >> +{ >> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); >> + >> + if (!drm_enc) { >> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >> + return -EINVAL; >> + } >> + >> + return dpu_encoder_get_crc(drm_enc); >> +} >> + >> static int dpu_crtc_get_crc(struct drm_crtc *crtc) >> { >> struct dpu_crtc_state *crtc_state; >> @@ -227,6 +265,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc) >> if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >> return dpu_crtc_get_lm_crc(crtc, crtc_state); >> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF) >> + return dpu_crtc_get_encoder_crc(crtc); >> + >> return 0; >> } >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >> index b8785c394fcc..a60af034905d 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.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. >> * Copyright (C) 2013 Red Hat >> * Author: Rob Clark <robdclark@gmail.com> >> @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data { >> * enum dpu_crtc_crc_source: CRC source >> * @DPU_CRTC_CRC_SOURCE_NONE: no source set >> * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer >> + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface >> * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source >> */ >> enum dpu_crtc_crc_source { >> DPU_CRTC_CRC_SOURCE_NONE = 0, >> DPU_CRTC_CRC_SOURCE_LAYER_MIXER, >> + DPU_CRTC_CRC_SOURCE_INTF, >> DPU_CRTC_CRC_SOURCE_MAX, >> DPU_CRTC_CRC_SOURCE_INVALID = -1 >> }; >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 52516eb20cb8..7740515f462d 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -14,6 +14,7 @@ >> #include <drm/drm_crtc.h> >> #include <drm/drm_file.h> >> +#include <drm/drm_vblank.h> >> #include <drm/drm_probe_helper.h> >> #include "msm_drv.h" >> @@ -225,6 +226,66 @@ bool dpu_encoder_is_widebus_enabled(const struct >> drm_encoder *drm_enc) >> return dpu_enc->wide_bus_en; >> } >> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc) >> +{ >> + struct dpu_encoder_virt *dpu_enc; >> + >> + dpu_enc = to_dpu_encoder_virt(drm_enc); >> + >> + return dpu_enc->num_phys_encs; >> +} >> + >> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) >> +{ >> + struct dpu_encoder_virt *dpu_enc; >> + >> + int i; >> + >> + dpu_enc = to_dpu_encoder_virt(drm_enc); >> + >> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >> + >> + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) >> + continue; > > Does WB support CRC? AFAIK, no. > >> + >> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); >> + } >> +} >> + >> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) >> +{ >> + struct dpu_encoder_virt *dpu_enc; >> + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; >> + >> + int i, rc; >> + >> + if (!drm_enc->crtc) { >> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index); >> + return -EINVAL; >> + } >> + >> + dpu_enc = to_dpu_encoder_virt(drm_enc); >> + >> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >> + >> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr) >> + continue; >> + >> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[i]); > > This doesn't look fully correct. Do we need to skip the indices for the > phys without a backing hw_intf? Sorry if I'm misunderstanding your question, but don't we need to have a backing hw_intf (and skip if there isn't any) since the methods for collecting/setting MISR registers is within the hw_intf? > >> + > > Extra empty line. Noted > >> + if (rc) { >> + if (rc != -ENODATA) > > Do we need to handle ENODATA in any specific way (like zeroing the > crcs[i])? If not, I'd suggest to drop this return code. Let's make an > error always an error. This is a carry-over from this change [1]. We wanted to have the ENODATA check to avoid spamming the driver debug logs when CRC is disabled for this block. [1] https://gitlab.freedesktop.org/drm/msm/-/commit/3ce8bdca394fc606b55e7c5ed779d171aaae5d09 Thanks, Jessica Zhang > >> + DRM_DEBUG_DRIVER("MISR read failed\n"); >> + return rc; >> + } >> + } >> + >> + return drm_crtc_add_crc_entry(drm_enc->crtc, true, >> + drm_crtc_accurate_vblank_count(drm_enc->crtc), crcs); >> +} >> + >> static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, >> unsigned bpc) >> { >> struct dpu_hw_dither_cfg dither_cfg = { 0 }; >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> index 781d41c91994..8345599dd01a 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.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-2018, The Linux Foundation. All rights reserved. >> * Copyright (C) 2013 Red Hat >> * Author: Rob Clark <robdclark@gmail.com> >> @@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct >> drm_encoder *drm_enc); >> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); >> +/** >> + * dpu_encoder_get_num_phys - get number of physical encoders >> contained in virtual >> + * encoder >> + * @drm_enc: Pointer to previously created drm encoder structure >> + * Returns: Number of physical encoders for given drm encoder >> + */ >> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc); >> + >> +/** >> + * dpu_encoder_setup_misr - enable misr calculations >> + * @drm_enc: Pointer to previously created drm encoder structure >> + */ >> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); >> + >> +/** >> + * dpu_encoder_get_crc - get the crc value from interface blocks >> + * @drm_enc: Pointer to previously created drm encoder structure >> + * Returns: 0 on success, error otherwise >> + */ >> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc); >> + >> /** >> * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC >> merge topology. >> * @drm_enc: Pointer to previously created drm encoder structure > > > -- > With best wishes > Dmitry
On 28/05/2022 01:23, Jessica Zhang wrote: > > > On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote: >> On 27/05/2022 21:54, Jessica Zhang wrote: >>> Add support for writing CRC values for the interface block to >>> the debugfs by calling the necessary MISR setup/collect methods. >>> >>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ >>> 4 files changed, 128 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> index ae09466663cf..e830fb1e910d 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source >>> dpu_crtc_parse_crc_source(const char *src_name) >>> if (!strcmp(src_name, "auto") || >>> !strcmp(src_name, "lm")) >>> return DPU_CRTC_CRC_SOURCE_LAYER_MIXER; >>> + if (!strcmp(src_name, "intf")) >>> + return DPU_CRTC_CRC_SOURCE_INTF; >>> return DPU_CRTC_CRC_SOURCE_INVALID; >>> } >>> @@ -94,8 +96,18 @@ static int dpu_crtc_verify_crc_source(struct >>> drm_crtc *crtc, >>> return -EINVAL; >>> } >>> - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >>> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) { >>> *values_cnt = crtc_state->num_mixers; >>> + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) { >>> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); >> >> Let's do this correctly from the beginning. The CRTC can drive several >> encoders. Do we want to get CRC from all of them or just from the >> first one? > > In the case of multiple encoders, it would be better to collect CRCs > from all of them. Then this should become a loop. > >> >>> + >>> + if (!drm_enc) { >>> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >>> + return -ENODATA; >>> + } >>> + >>> + *values_cnt = dpu_encoder_get_num_phys(drm_enc); >>> + } >>> return 0; >>> } >>> @@ -116,6 +128,18 @@ static void dpu_crtc_setup_lm_misr(struct >>> dpu_crtc_state *crtc_state) >>> } >>> } >>> +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) >>> +{ >>> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); >>> + >>> + if (!drm_enc) { >>> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >>> + return; >>> + } >>> + >>> + dpu_encoder_setup_misr(drm_enc); >>> +} >>> + >>> 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); >>> @@ -164,6 +188,8 @@ static int dpu_crtc_set_crc_source(struct >>> drm_crtc *crtc, const char *src_name) >>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >>> dpu_crtc_setup_lm_misr(crtc_state); >>> + else if (source == DPU_CRTC_CRC_SOURCE_INTF) >>> + dpu_crtc_setup_encoder_misr(crtc); >>> cleanup: >>> drm_modeset_unlock(&crtc->mutex); >>> @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc >>> *crtc, struct dpu_crtc_state *crt >>> drm_crtc_accurate_vblank_count(crtc), crcs); >>> } >>> +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) >>> +{ >>> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); >>> + >>> + if (!drm_enc) { >>> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >>> + return -EINVAL; >>> + } >>> + >>> + return dpu_encoder_get_crc(drm_enc); >>> +} >>> + >>> static int dpu_crtc_get_crc(struct drm_crtc *crtc) >>> { >>> struct dpu_crtc_state *crtc_state; >>> @@ -227,6 +265,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc) >>> if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >>> return dpu_crtc_get_lm_crc(crtc, crtc_state); >>> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF) >>> + return dpu_crtc_get_encoder_crc(crtc); >>> + >>> return 0; >>> } >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >>> index b8785c394fcc..a60af034905d 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.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. >>> * Copyright (C) 2013 Red Hat >>> * Author: Rob Clark <robdclark@gmail.com> >>> @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data { >>> * enum dpu_crtc_crc_source: CRC source >>> * @DPU_CRTC_CRC_SOURCE_NONE: no source set >>> * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer >>> + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface >>> * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source >>> */ >>> enum dpu_crtc_crc_source { >>> DPU_CRTC_CRC_SOURCE_NONE = 0, >>> DPU_CRTC_CRC_SOURCE_LAYER_MIXER, >>> + DPU_CRTC_CRC_SOURCE_INTF, >>> DPU_CRTC_CRC_SOURCE_MAX, >>> DPU_CRTC_CRC_SOURCE_INVALID = -1 >>> }; >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 52516eb20cb8..7740515f462d 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -14,6 +14,7 @@ >>> #include <drm/drm_crtc.h> >>> #include <drm/drm_file.h> >>> +#include <drm/drm_vblank.h> >>> #include <drm/drm_probe_helper.h> >>> #include "msm_drv.h" >>> @@ -225,6 +226,66 @@ bool dpu_encoder_is_widebus_enabled(const struct >>> drm_encoder *drm_enc) >>> return dpu_enc->wide_bus_en; >>> } >>> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc) >>> +{ >>> + struct dpu_encoder_virt *dpu_enc; >>> + >>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>> + >>> + return dpu_enc->num_phys_encs; >>> +} >>> + >>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) >>> +{ >>> + struct dpu_encoder_virt *dpu_enc; >>> + >>> + int i; >>> + >>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>> + >>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >>> + >>> + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) >>> + continue; >> >> Does WB support CRC? > > AFAIK, no. > >> >>> + >>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); >>> + } >>> +} >>> + >>> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) >>> +{ >>> + struct dpu_encoder_virt *dpu_enc; >>> + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; >>> + >>> + int i, rc; >>> + >>> + if (!drm_enc->crtc) { >>> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index); >>> + return -EINVAL; >>> + } >>> + >>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>> + >>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >>> + >>> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr) >>> + continue; >>> + >>> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[i]); >> >> This doesn't look fully correct. Do we need to skip the indices for >> the phys without a backing hw_intf? > > Sorry if I'm misunderstanding your question, but don't we need to have a > backing hw_intf (and skip if there isn't any) since the methods for > collecting/setting MISR registers is within the hw_intf? Yes. So the question if we should skip the phys and leave the crcs[i] untouched, skip the phys and sset crcs[i] to 0 or change dpu_crtc_parse_crc_source() to return the number of intf-backed phys_enc's and do not skip any crcs[i]. > >> >>> + >> >> Extra empty line. > > Noted > >> >>> + if (rc) { >>> + if (rc != -ENODATA) >> >> Do we need to handle ENODATA in any specific way (like zeroing the >> crcs[i])? If not, I'd suggest to drop this return code. Let's make an >> error always an error. > > This is a carry-over from this change [1]. We wanted to have the ENODATA > check to avoid spamming the driver debug logs when CRC is disabled for > this block. > > [1] > https://gitlab.freedesktop.org/drm/msm/-/commit/3ce8bdca394fc606b55e7c5ed779d171aaae5d09 Thanks for the reminder. I commented this in the previous patch now.
On 6/2/2022 3:51 PM, Dmitry Baryshkov wrote: > On 28/05/2022 01:23, Jessica Zhang wrote: >> >> >> On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote: >>> On 27/05/2022 21:54, Jessica Zhang wrote: >>>> Add support for writing CRC values for the interface block to >>>> the debugfs by calling the necessary MISR setup/collect methods. >>>> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 >>>> +++++++++++++++++++++ >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ >>>> 4 files changed, 128 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> index ae09466663cf..e830fb1e910d 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source >>>> dpu_crtc_parse_crc_source(const char *src_name) >>>> if (!strcmp(src_name, "auto") || >>>> !strcmp(src_name, "lm")) >>>> return DPU_CRTC_CRC_SOURCE_LAYER_MIXER; >>>> + if (!strcmp(src_name, "intf")) >>>> + return DPU_CRTC_CRC_SOURCE_INTF; >>>> return DPU_CRTC_CRC_SOURCE_INVALID; >>>> } >>>> @@ -94,8 +96,18 @@ static int dpu_crtc_verify_crc_source(struct >>>> drm_crtc *crtc, >>>> return -EINVAL; >>>> } >>>> - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >>>> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) { >>>> *values_cnt = crtc_state->num_mixers; >>>> + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) { >>>> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); >>> >>> Let's do this correctly from the beginning. The CRTC can drive >>> several encoders. Do we want to get CRC from all of them or just from >>> the first one? >> >> In the case of multiple encoders, it would be better to collect CRCs >> from all of them. > > Then this should become a loop. Ah, I see what you mean. Yea, I can do that. > >> >>> >>>> + >>>> + if (!drm_enc) { >>>> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >>>> + return -ENODATA; >>>> + } >>>> + >>>> + *values_cnt = dpu_encoder_get_num_phys(drm_enc); >>>> + } >>>> return 0; >>>> } >>>> @@ -116,6 +128,18 @@ static void dpu_crtc_setup_lm_misr(struct >>>> dpu_crtc_state *crtc_state) >>>> } >>>> } >>>> +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) >>>> +{ >>>> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); >>>> + >>>> + if (!drm_enc) { >>>> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >>>> + return; >>>> + } >>>> + >>>> + dpu_encoder_setup_misr(drm_enc); >>>> +} >>>> + >>>> 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); >>>> @@ -164,6 +188,8 @@ static int dpu_crtc_set_crc_source(struct >>>> drm_crtc *crtc, const char *src_name) >>>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >>>> dpu_crtc_setup_lm_misr(crtc_state); >>>> + else if (source == DPU_CRTC_CRC_SOURCE_INTF) >>>> + dpu_crtc_setup_encoder_misr(crtc); >>>> cleanup: >>>> drm_modeset_unlock(&crtc->mutex); >>>> @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc >>>> *crtc, struct dpu_crtc_state *crt >>>> drm_crtc_accurate_vblank_count(crtc), crcs); >>>> } >>>> +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) >>>> +{ >>>> + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); >>>> + >>>> + if (!drm_enc) { >>>> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return dpu_encoder_get_crc(drm_enc); >>>> +} >>>> + >>>> static int dpu_crtc_get_crc(struct drm_crtc *crtc) >>>> { >>>> struct dpu_crtc_state *crtc_state; >>>> @@ -227,6 +265,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc) >>>> if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) >>>> return dpu_crtc_get_lm_crc(crtc, crtc_state); >>>> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF) >>>> + return dpu_crtc_get_encoder_crc(crtc); >>>> + >>>> return 0; >>>> } >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >>>> index b8785c394fcc..a60af034905d 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.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. >>>> * Copyright (C) 2013 Red Hat >>>> * Author: Rob Clark <robdclark@gmail.com> >>>> @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data { >>>> * enum dpu_crtc_crc_source: CRC source >>>> * @DPU_CRTC_CRC_SOURCE_NONE: no source set >>>> * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer >>>> + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface >>>> * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source >>>> */ >>>> enum dpu_crtc_crc_source { >>>> DPU_CRTC_CRC_SOURCE_NONE = 0, >>>> DPU_CRTC_CRC_SOURCE_LAYER_MIXER, >>>> + DPU_CRTC_CRC_SOURCE_INTF, >>>> DPU_CRTC_CRC_SOURCE_MAX, >>>> DPU_CRTC_CRC_SOURCE_INVALID = -1 >>>> }; >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> index 52516eb20cb8..7740515f462d 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> @@ -14,6 +14,7 @@ >>>> #include <drm/drm_crtc.h> >>>> #include <drm/drm_file.h> >>>> +#include <drm/drm_vblank.h> >>>> #include <drm/drm_probe_helper.h> >>>> #include "msm_drv.h" >>>> @@ -225,6 +226,66 @@ bool dpu_encoder_is_widebus_enabled(const >>>> struct drm_encoder *drm_enc) >>>> return dpu_enc->wide_bus_en; >>>> } >>>> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc) >>>> +{ >>>> + struct dpu_encoder_virt *dpu_enc; >>>> + >>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>> + >>>> + return dpu_enc->num_phys_encs; >>>> +} >>>> + >>>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) >>>> +{ >>>> + struct dpu_encoder_virt *dpu_enc; >>>> + >>>> + int i; >>>> + >>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>> + >>>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >>>> + >>>> + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) >>>> + continue; >>> >>> Does WB support CRC? >> >> AFAIK, no. >> >>> >>>> + >>>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); >>>> + } >>>> +} >>>> + >>>> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) >>>> +{ >>>> + struct dpu_encoder_virt *dpu_enc; >>>> + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; >>>> + >>>> + int i, rc; >>>> + >>>> + if (!drm_enc->crtc) { >>>> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>> + >>>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >>>> + >>>> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr) >>>> + continue; >>>> + >>>> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[i]); >>> >>> This doesn't look fully correct. Do we need to skip the indices for >>> the phys without a backing hw_intf? >> >> Sorry if I'm misunderstanding your question, but don't we need to have >> a backing hw_intf (and skip if there isn't any) since the methods for >> collecting/setting MISR registers is within the hw_intf? > > Yes. So the question if we should skip the phys and leave the crcs[i] > untouched, skip the phys and sset crcs[i] to 0 or change > dpu_crtc_parse_crc_source() to return the number of intf-backed > phys_enc's and do not skip any crcs[i]. Thanks for the clarification. Is it possible to hit a case where a phys_encoder won't have a corresponding hw_intf? AFAIK, it seems guaranteed that a phys_encoder will have an hw_intf since dpu_encoder_setup_display will skip incrementing num_phys_encs if dpu_encoder_get_intf fails [1]. [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2263 Thanks, Jessica Zhang > > >> >>> >>>> + >>> >>> Extra empty line. >> >> Noted >> >>> >>>> + if (rc) { >>>> + if (rc != -ENODATA) >>> >>> Do we need to handle ENODATA in any specific way (like zeroing the >>> crcs[i])? If not, I'd suggest to drop this return code. Let's make an >>> error always an error. >> >> This is a carry-over from this change [1]. We wanted to have the >> ENODATA check to avoid spamming the driver debug logs when CRC is >> disabled for this block. >> >> [1] >> https://gitlab.freedesktop.org/drm/msm/-/commit/3ce8bdca394fc606b55e7c5ed779d171aaae5d09 > > > Thanks for the reminder. I commented this in the previous patch now. > > > > -- > With best wishes > Dmitry
On Fri, 3 Jun 2022 at 04:02, Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > On 6/2/2022 3:51 PM, Dmitry Baryshkov wrote: > > On 28/05/2022 01:23, Jessica Zhang wrote: > >> On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote: > >>> On 27/05/2022 21:54, Jessica Zhang wrote: > >>>> Add support for writing CRC values for the interface block to > >>>> the debugfs by calling the necessary MISR setup/collect methods. > >>>> > >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> [skipped] > >>>> + > >>>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); > >>>> + } > >>>> +} > >>>> + > >>>> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) > >>>> +{ > >>>> + struct dpu_encoder_virt *dpu_enc; > >>>> + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; > >>>> + > >>>> + int i, rc; > >>>> + > >>>> + if (!drm_enc->crtc) { > >>>> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); > >>>> + > >>>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { > >>>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > >>>> + > >>>> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr) > >>>> + continue; > >>>> + > >>>> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[i]); > >>> > >>> This doesn't look fully correct. Do we need to skip the indices for > >>> the phys without a backing hw_intf? > >> > >> Sorry if I'm misunderstanding your question, but don't we need to have > >> a backing hw_intf (and skip if there isn't any) since the methods for > >> collecting/setting MISR registers is within the hw_intf? > > > > Yes. So the question if we should skip the phys and leave the crcs[i] > > untouched, skip the phys and sset crcs[i] to 0 or change > > dpu_crtc_parse_crc_source() to return the number of intf-backed > > phys_enc's and do not skip any crcs[i]. > > Thanks for the clarification. > > Is it possible to hit a case where a phys_encoder won't have a > corresponding hw_intf? > > AFAIK, it seems guaranteed that a phys_encoder will have an hw_intf > since dpu_encoder_setup_display will skip incrementing num_phys_encs if > dpu_encoder_get_intf fails [1]. WB encoders won't have hw_intf. The code checks that either get_intf or get_wb succeeds. > > [1] > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2263
On 6/3/2022 12:02 AM, Dmitry Baryshkov wrote: > On Fri, 3 Jun 2022 at 04:02, Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >> On 6/2/2022 3:51 PM, Dmitry Baryshkov wrote: >>> On 28/05/2022 01:23, Jessica Zhang wrote: >>>> On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote: >>>>> On 27/05/2022 21:54, Jessica Zhang wrote: >>>>>> Add support for writing CRC values for the interface block to >>>>>> the debugfs by calling the necessary MISR setup/collect methods. >>>>>> >>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > > [skipped] > >>>>>> + >>>>>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) >>>>>> +{ >>>>>> + struct dpu_encoder_virt *dpu_enc; >>>>>> + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; >>>>>> + >>>>>> + int i, rc; >>>>>> + >>>>>> + if (!drm_enc->crtc) { >>>>>> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>>>> + >>>>>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>>>>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >>>>>> + >>>>>> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr) >>>>>> + continue; >>>>>> + >>>>>> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[i]); >>>>> >>>>> This doesn't look fully correct. Do we need to skip the indices for >>>>> the phys without a backing hw_intf? >>>> >>>> Sorry if I'm misunderstanding your question, but don't we need to have >>>> a backing hw_intf (and skip if there isn't any) since the methods for >>>> collecting/setting MISR registers is within the hw_intf? >>> >>> Yes. So the question if we should skip the phys and leave the crcs[i] >>> untouched, skip the phys and sset crcs[i] to 0 or change >>> dpu_crtc_parse_crc_source() to return the number of intf-backed >>> phys_enc's and do not skip any crcs[i]. >> >> Thanks for the clarification. >> >> Is it possible to hit a case where a phys_encoder won't have a >> corresponding hw_intf? >> >> AFAIK, it seems guaranteed that a phys_encoder will have an hw_intf >> since dpu_encoder_setup_display will skip incrementing num_phys_encs if >> dpu_encoder_get_intf fails [1]. > > WB encoders won't have hw_intf. The code checks that either get_intf > or get_wb succeeds. Got it, I see your point. I'll change the values_cnt to only include the intf-backed phys_encoders then. Thanks, Jessica Zhang > >> >> [1] >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2263 > > > -- > 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 ae09466663cf..e830fb1e910d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name) if (!strcmp(src_name, "auto") || !strcmp(src_name, "lm")) return DPU_CRTC_CRC_SOURCE_LAYER_MIXER; + if (!strcmp(src_name, "intf")) + return DPU_CRTC_CRC_SOURCE_INTF; return DPU_CRTC_CRC_SOURCE_INVALID; } @@ -94,8 +96,18 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc, return -EINVAL; } - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) { *values_cnt = crtc_state->num_mixers; + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) { + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); + + if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -ENODATA; + } + + *values_cnt = dpu_encoder_get_num_phys(drm_enc); + } return 0; } @@ -116,6 +128,18 @@ static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state) } } +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc) +{ + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); + + if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return; + } + + dpu_encoder_setup_misr(drm_enc); +} + 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); @@ -164,6 +188,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name) if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) dpu_crtc_setup_lm_misr(crtc_state); + else if (source == DPU_CRTC_CRC_SOURCE_INTF) + dpu_crtc_setup_encoder_misr(crtc); cleanup: drm_modeset_unlock(&crtc->mutex); @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt drm_crtc_accurate_vblank_count(crtc), crcs); } +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) +{ + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); + + if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -EINVAL; + } + + return dpu_encoder_get_crc(drm_enc); +} + static int dpu_crtc_get_crc(struct drm_crtc *crtc) { struct dpu_crtc_state *crtc_state; @@ -227,6 +265,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc) if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) return dpu_crtc_get_lm_crc(crtc, crtc_state); + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF) + return dpu_crtc_get_encoder_crc(crtc); + return 0; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index b8785c394fcc..a60af034905d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.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. * Copyright (C) 2013 Red Hat * Author: Rob Clark <robdclark@gmail.com> @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data { * enum dpu_crtc_crc_source: CRC source * @DPU_CRTC_CRC_SOURCE_NONE: no source set * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source */ enum dpu_crtc_crc_source { DPU_CRTC_CRC_SOURCE_NONE = 0, DPU_CRTC_CRC_SOURCE_LAYER_MIXER, + DPU_CRTC_CRC_SOURCE_INTF, DPU_CRTC_CRC_SOURCE_MAX, DPU_CRTC_CRC_SOURCE_INVALID = -1 }; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 52516eb20cb8..7740515f462d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -14,6 +14,7 @@ #include <drm/drm_crtc.h> #include <drm/drm_file.h> +#include <drm/drm_vblank.h> #include <drm/drm_probe_helper.h> #include "msm_drv.h" @@ -225,6 +226,66 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) return dpu_enc->wide_bus_en; } +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc) +{ + struct dpu_encoder_virt *dpu_enc; + + dpu_enc = to_dpu_encoder_virt(drm_enc); + + return dpu_enc->num_phys_encs; +} + +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc) +{ + struct dpu_encoder_virt *dpu_enc; + + int i; + + dpu_enc = to_dpu_encoder_virt(drm_enc); + + for (i = 0; i < dpu_enc->num_phys_encs; i++) { + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; + + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr) + continue; + + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); + } +} + +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) +{ + struct dpu_encoder_virt *dpu_enc; + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; + + int i, rc; + + if (!drm_enc->crtc) { + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index); + return -EINVAL; + } + + dpu_enc = to_dpu_encoder_virt(drm_enc); + + for (i = 0; i < dpu_enc->num_phys_encs; i++) { + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; + + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr) + continue; + + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[i]); + + if (rc) { + if (rc != -ENODATA) + DRM_DEBUG_DRIVER("MISR read failed\n"); + return rc; + } + } + + return drm_crtc_add_crc_entry(drm_enc->crtc, true, + drm_crtc_accurate_vblank_count(drm_enc->crtc), crcs); +} + static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc) { struct dpu_hw_dither_cfg dither_cfg = { 0 }; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 781d41c91994..8345599dd01a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.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-2018, The Linux Foundation. All rights reserved. * Copyright (C) 2013 Red Hat * Author: Rob Clark <robdclark@gmail.com> @@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc); bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); +/** + * dpu_encoder_get_num_phys - get number of physical encoders contained in virtual + * encoder + * @drm_enc: Pointer to previously created drm encoder structure + * Returns: Number of physical encoders for given drm encoder + */ +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc); + +/** + * dpu_encoder_setup_misr - enable misr calculations + * @drm_enc: Pointer to previously created drm encoder structure + */ +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder); + +/** + * dpu_encoder_get_crc - get the crc value from interface blocks + * @drm_enc: Pointer to previously created drm encoder structure + * Returns: 0 on success, error otherwise + */ +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc); + /** * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology. * @drm_enc: Pointer to previously created drm encoder structure
Add support for writing CRC values for the interface block to the debugfs by calling the necessary MISR setup/collect methods. Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ 4 files changed, 128 insertions(+), 1 deletion(-)