On Tue, Jan 28, 2025 at 07:20:34PM -0800, Jessica Zhang wrote:
> From: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
>
> All resource allocation is centered around the LMs. Then other blocks
> (except DSCs) are allocated basing on the LMs that was selected, and LM
> powers up the CRTC rather than the encoder.
>
> Moreover if at some point the driver supports encoder cloning,
> allocating resources from the encoder will be incorrect, as all clones
> will have different encoder IDs, while LMs are to be shared by these
> encoders.
>
> In addition, move mode_changed() to dpu_crtc as encoder no longer has
> access to topology information
>
> Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
> [quic_abhin...@quicinc.com: Refactored resource allocation for CDM]
> Signed-off-by: Abhinav Kumar <quic_abhin...@quicinc.com>
> [quic_jessz...@quicinc.com: Changed to grabbing exising global state]
> Signed-off-by: Jessica Zhang <quic_jessz...@quicinc.com>
>
> ---
> Changes in v5:
> - Reordered to prevent breaking CI and upon partial applciation
> - Moved mode_changed() from dpu_encoder to dpu_crtc
> - Dropped dpu_encoder_needs_dsc_merge() refactor to clean up commit
> - In dpu_encoder_update_topology(), grab DSC config using dpu_encoder
> helper as dpu_encoder->dsc hasn't been set yet
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 79 +++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 174
> +++++++++-------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 11 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 19 +--
> 5 files changed, 144 insertions(+), 141 deletions(-)
>
> -/**
> - * dpu_encoder_virt_check_mode_changed: check if full modeset is required
> - * @drm_enc: Pointer to drm encoder structure
> - * @crtc_state: Corresponding CRTC state to be checked
> - * @conn_state: Corresponding Connector's state to be checked
> - *
> - * Check if the changes in the object properties demand full mode set.
> - */
> -int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
> - struct drm_crtc_state *crtc_state,
> - struct drm_connector_state *conn_state)
> +bool dpu_encoder_needs_modeset(struct drm_encoder *drm_enc, struct
> drm_atomic_state *state)
> {
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> + struct drm_framebuffer *fb;
> struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> - struct msm_display_topology topology;
> -
> - DPU_DEBUG_ENC(dpu_enc, "\n");
> -
> - /* Using mode instead of adjusted_mode as it wasn't computed yet */
> - topology = dpu_encoder_get_topology(dpu_enc, &crtc_state->mode,
> crtc_state, conn_state);
> -
> - if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> - crtc_state->mode_changed = true;
> - else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> - crtc_state->mode_changed = true;
> -
> - return 0;
> -}
> -
> -static int dpu_encoder_virt_atomic_check(
> - struct drm_encoder *drm_enc,
> - struct drm_crtc_state *crtc_state,
> - struct drm_connector_state *conn_state)
> -{
> - struct dpu_encoder_virt *dpu_enc;
> - struct msm_drm_private *priv;
> - struct dpu_kms *dpu_kms;
> - struct drm_display_mode *adj_mode;
> - struct msm_display_topology topology;
> - struct dpu_global_state *global_state;
> - int ret = 0;
> -
> - if (!drm_enc || !crtc_state || !conn_state) {
> - DPU_ERROR("invalid arg(s), drm_enc %d, crtc/conn state %d/%d\n",
> - drm_enc != NULL, crtc_state != NULL, conn_state
> != NULL);
> - return -EINVAL;
> - }
> -
> - dpu_enc = to_dpu_encoder_virt(drm_enc);
> - DPU_DEBUG_ENC(dpu_enc, "\n");
> -
> - priv = drm_enc->dev->dev_private;
> - dpu_kms = to_dpu_kms(priv->kms);
> - adj_mode = &crtc_state->adjusted_mode;
> - global_state = dpu_kms_get_global_state(crtc_state->state);
> - if (IS_ERR(global_state))
> - return PTR_ERR(global_state);
>
> - trace_dpu_enc_atomic_check(DRMID(drm_enc));
> + if (!drm_enc || !state)
> + return false;
>
> - topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state,
> conn_state);
> + connector = drm_atomic_get_new_connector_for_encoder(state, drm_enc);
> + if (!connector)
> + return false;
>
> - /*
> - * Release and Allocate resources on every modeset
> - */
> - if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> - dpu_rm_release(global_state, drm_enc);
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
>
> - if (crtc_state->enable)
> - ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> - drm_enc, crtc_state, &topology);
> + if (dpu_enc->disp_info.intf_type == INTF_WB &&
> conn_state->writeback_job) {
> + fb = conn_state->writeback_job->fb;
> + if (fb && MSM_FORMAT_IS_YUV(msm_framebuffer_format(fb))) {
> + if (!dpu_enc->cur_master->hw_cdm)
> + return true;
> + } else {
> + if (dpu_enc->cur_master->hw_cdm)
> + return true;
> + }
Nit: this is duplicating a part of the dpu_encoder_update_topology(). It
would be nice to have a comment here. If there is no need for a new
versoion, I can probably write something when applying.
> }
>
> - trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
> -
> - return ret;
> + return false;
> }
>
> static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt
> *dpu_enc,
> @@ -2612,7 +2545,6 @@ static const struct drm_encoder_helper_funcs
> dpu_encoder_helper_funcs = {
> .atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
> .atomic_disable = dpu_encoder_virt_atomic_disable,
> .atomic_enable = dpu_encoder_virt_atomic_enable,
> - .atomic_check = dpu_encoder_virt_atomic_check,
> };
>
> static const struct drm_encoder_funcs dpu_encoder_funcs = {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index
> da133ee4701a329f566f6f9a7255f2f6d050f891..b0ac10ebd02c2b63e6f6f9010a22cdace931cf3b
> 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -80,6 +80,13 @@ int dpu_encoder_get_crc(const struct drm_encoder *drm_enc,
> u32 *crcs, int pos);
>
> bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc);
>
> +void dpu_encoder_update_topology(struct drm_encoder *drm_enc,
> + struct msm_display_topology *topology,
> + struct drm_atomic_state *state,
> + const struct drm_display_mode *adj_mode);
> +
> +bool dpu_encoder_needs_modeset(struct drm_encoder *drm_enc, struct
> drm_atomic_state *state);
> +
> void dpu_encoder_prepare_wb_job(struct drm_encoder *drm_enc,
> struct drm_writeback_job *job);
>
> @@ -88,8 +95,4 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc,
>
> bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc);
>
> -int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
> - struct drm_crtc_state *crtc_state,
> - struct drm_connector_state *conn_state);
> -
> #endif /* __DPU_ENCODER_H__ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index
> 5ce06e25990cb70284d3c3f04ac1e1e1bed6142a..c6b3b2e147b4c61ec93db4a9f01d5a288d2b9eb2
> 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -449,24 +449,11 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
> static int dpu_kms_check_mode_changed(struct msm_kms *kms, struct
> drm_atomic_state *state)
> {
> struct drm_crtc_state *new_crtc_state;
> - struct drm_connector *connector;
> - struct drm_connector_state *new_conn_state;
> + struct drm_crtc *crtc;
> int i;
>
> - for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> - struct drm_encoder *encoder;
> -
> - WARN_ON(!!new_conn_state->best_encoder !=
> !!new_conn_state->crtc);
> -
> - if (!new_conn_state->crtc || !new_conn_state->best_encoder)
> - continue;
> -
> - new_crtc_state = drm_atomic_get_new_crtc_state(state,
> new_conn_state->crtc);
> -
> - encoder = new_conn_state->best_encoder;
> -
> - dpu_encoder_virt_check_mode_changed(encoder, new_crtc_state,
> new_conn_state);
> - }
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> + dpu_crtc_check_mode_changed(new_crtc_state);
>
> return 0;
> }
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
