On 1/29/2025 2:04 PM, Dmitry Baryshkov wrote:
On Tue, Jan 28, 2025 at 07:20:34PM -0800, Jessica Zhang wrote:From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 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.baryshkov@xxxxxxxxxx> [quic_abhinavk@xxxxxxxxxxx: Refactored resource allocation for CDM] Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> [quic_jesszhan@xxxxxxxxxxx: Changed to grabbing exising global state] Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> --- 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.
Sure, I can add a note that we need to duplicate these checks in *_needs_modeset() since topology info is not stored in the encoder or crtc
Thanks, Jessica Zhang
}- 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
