On Thu, Feb 06, 2025 at 03:49:53PM -0800, Jessica Zhang wrote:
>
>
> 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
Yes, please. I was thinking about having a helper function, but I don't
think that it makes real sense.
--
With best wishes
Dmitry
