Re: [PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function

[Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>]

On 3/1/2023 2:13 AM, Marijn Suijten wrote:
> On 2023-03-01 11:08:16, Marijn Suijten wrote:
> > On 2023-02-21 10:42:55, Jessica Zhang wrote:
> > > Now that the TE setup has been moved to prepare_for_kickoff(),  we have
> > > not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit()
> >
> > s/not/no
> >
> > > do nothing. Remove prepare_commit() from DPU driver.
> >
> > And again, this:
> >
> > > Changes in V3:
> > > - Reworded commit message to be more clear
> > > - Corrected spelling mistake in commit message
> > >
> > > Changes in V4:
> > > - Reworded commit message for clarity
> >
> > ... should go below the cut.
> >
> > > Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
> >
> > With the above two issues fixed:
> >
> > Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> >
> > > ---
> > >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 -------------------
> > >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  7 -------
> > >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 21 ---------------------
> > >   3 files changed, 47 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index dcceed91aed8..35e120b5ef53 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
> > >   	ctl->ops.clear_pending_flush(ctl);
> > >   }
> > >   
> > > -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> > > -{
> > > -	struct dpu_encoder_virt *dpu_enc;
> > > -	struct dpu_encoder_phys *phys;
> > > -	int i;
> > > -
> > > -	if (!drm_enc) {
> > > -		DPU_ERROR("invalid encoder\n");
> > > -		return;
> > > -	}
> > > -	dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > -
> > > -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > > -		phys = dpu_enc->phys_encs[i];
> > > -		if (phys->ops.prepare_commit)
> > > -			phys->ops.prepare_commit(phys);
>
> In hindsight, Dmitry asked in v2 to remove prepare_commit from
> dpu_encoder_phys_ops (and its documentation comment) in
> dpu_encoder_phys.h, but that has not happened yet.  Can we do that in a
> v5?

Ah, forgot to include that change. Will add it in the v5. Thanks for 
catching it!

- Jessica Zhang

> - Marijn
>
> > > -	}
> > > -}
> > > -
> > >   #ifdef CONFIG_DEBUG_FS
> > >   static int _dpu_encoder_status_show(struct seq_file *s, void *data)
> > >   {
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > index 9e7236ef34e6..2c9ef8d1b877 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init(
> > >   int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> > >   		struct msm_display_info *disp_info);
> > >   
> > > -/**
> > > - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
> > > - *	atomic commit, before any registers are written
> > > - * @drm_enc:    Pointer to previously created drm encoder structure
> > > - */
> > > -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> > > -
> > >   /**
> > >    * dpu_encoder_set_idle_timeout - set the idle timeout for video
> > >    *                    and command mode encoders.
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > index 165958d47ec6..6f7ddbf0d9b7 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
> > >   	return ktime_get();
> > >   }
> > >   
> > > -static void dpu_kms_prepare_commit(struct msm_kms *kms,
> > > -		struct drm_atomic_state *state)
> > > -{
> > > -	struct drm_crtc *crtc;
> > > -	struct drm_crtc_state *crtc_state;
> > > -	struct drm_encoder *encoder;
> > > -	int i;
> > > -
> > > -	if (!kms)
> > > -		return;
> > > -
> > > -	/* Call prepare_commit for all affected encoders */
> > > -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > -		drm_for_each_encoder_mask(encoder, crtc->dev,
> > > -					  crtc_state->encoder_mask) {
> > > -			dpu_encoder_prepare_commit(encoder);
> > > -		}
> > > -	}
> > > -}
> > > -
> > >   static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> > >   {
> > >   	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> > > @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = {
> > >   	.enable_commit   = dpu_kms_enable_commit,
> > >   	.disable_commit  = dpu_kms_disable_commit,
> > >   	.vsync_time      = dpu_kms_vsync_time,
> > > -	.prepare_commit  = dpu_kms_prepare_commit,
> > >   	.flush_commit    = dpu_kms_flush_commit,
> > >   	.wait_flush      = dpu_kms_wait_flush,
> > >   	.complete_commit = dpu_kms_complete_commit,
> > > --
> > > 2.39.2