[PATCH 3/8] drm/msm: dpu: Remove vblank_callback from encoder

Sean Paul sean at poorly.run
Wed Nov 14 15:13:23 UTC 2018


On Tue, Nov 13, 2018 at 04:47:22PM -0800, Jeykumar Sankaran wrote:
> On 2018-11-13 12:52, Sean Paul wrote:
> > From: Sean Paul <seanpaul at chromium.org>
> > 
> > The indirection of registering a callback and opaque pointer isn't real
> > useful when there's only one callsite. So instead of having the
> > vblank_cb registration, just give encoder a crtc and let it directly
> > call the vblank handler.
> > 
> > In a later patch, we'll make use of this further.
> > 
> > Signed-off-by: Sean Paul <seanpaul at chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  8 +++----
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 +++++
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 +++++++++++----------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++-----
> >  4 files changed, 26 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index adda0aa0cbaa..38119b4d4a80 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -291,9 +291,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> > drm_crtc *crtc)
> >  	return INTF_MODE_NONE;
> >  }
> > 
> > -static void dpu_crtc_vblank_cb(void *data)
> > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc)
> >  {
> > -	struct drm_crtc *crtc = (struct drm_crtc *)data;
> >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> 
> Since we are calling into a locally stored CRTC and not tracking disable.
> Should
> we check for crtc->enable before processing the callback?
> 
> I see vblank_on/off cant be called when CRTC is disabled with the rest
> of the patches in the series. But this callback is triggered by IRQ's
> context.

Hmm, yeah, I had assumed that we wouldn't have any interrupts after irq disable.
However it doesn't seem like that's the case.

That said, I don't think checking crtc->enable is quite what we want. In the
case of disable, the crtc is cleared from the encoder, so checking for
crtc != NULL would be better here. SGTY?

Now that I've dug into the vblank irq handling a bit in the encoder, it seems
like that could be moved to the crtc and a bunch of the encoder->crtc->encoder
bouncing around could be avoided. Off the top of your head, is there any reason
we couldn't move the vblank irq handling into crtc from encoder?

Sean

> 
> > 
> >  	/* keep statistics on vblank callback - with auto reset via
> > debugfs */
> > @@ -779,8 +778,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> >  						     DRMID(enc), enable,
> >  						     dpu_crtc);
> > 
> > -			dpu_encoder_register_vblank_callback(enc,
> > -					dpu_crtc_vblank_cb, (void *)crtc);
> > +			dpu_encoder_assign_crtc(enc, crtc);
> >  		}
> >  	} else {
> >  		list_for_each_entry(enc, &dev->mode_config.encoder_list,
> > head) {
> > @@ -791,7 +789,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
> >  						     DRMID(enc), enable,
> >  						     dpu_crtc);
> > 
> > -			dpu_encoder_register_vblank_callback(enc, NULL,
> > NULL);
> > +			dpu_encoder_assign_crtc(enc, NULL);
> >  		}
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > index 93d21a61a040..54595cc29be5 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct
> > drm_crtc *crtc)
> >   */
> >  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> > 
> > +/**
> > + * dpu_crtc_vblank_callback - called on vblank irq, issues completion
> > events
> > + * @crtc: Pointer to drm crtc object
> > + */
> > +void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
> > +
> >  /**
> >   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this
> > crtc
> >   * @crtc: Pointer to drm crtc object
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index d89ac520f7e6..fd6514f681ae 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -142,9 +142,11 @@ enum dpu_enc_rc_states {
> >   * @intfs_swapped	Whether or not the phys_enc interfaces have been
> > swapped
> >   *			for partial update right-only cases, such as
> > pingpong
> >   *			split where virtual pingpong does not generate
> > IRQs
> > - * @crtc_vblank_cb:	Callback into the upper layer / CRTC for
> > - *			notification of the VBLANK
> > - * @crtc_vblank_cb_data:	Data from upper layer for VBLANK
> > notification
> > + * @crtc:		Pointer to the currently assigned crtc. Normally
> > you
> > + *			would use crtc->state->encoder_mask to determine
> > the
> > + *			link between encoder/crtc. However in this case we
> > need
> > + *			to track crtc in the disable() hook which is
> > called
> > + *			_after_ encoder_mask is cleared.
> >   * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
> >   *				all CTL paths
> >   * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> > @@ -186,8 +188,7 @@ struct dpu_encoder_virt {
> > 
> >  	bool intfs_swapped;
> > 
> > -	void (*crtc_vblank_cb)(void *);
> > -	void *crtc_vblank_cb_data;
> > +	struct drm_crtc *crtc;
> > 
> >  	struct dentry *debugfs_root;
> >  	struct mutex enc_lock;
> > @@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct
> > drm_encoder *drm_enc,
> >  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> > 
> >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > -	if (dpu_enc->crtc_vblank_cb)
> > -		dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data);
> > +	if (dpu_enc->crtc)
> > +		dpu_crtc_vblank_callback(dpu_enc->crtc);
> >  	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> > 
> >  	atomic_inc(&phy_enc->vsync_cnt);
> > @@ -1262,15 +1263,14 @@ static void dpu_encoder_underrun_callback(struct
> > drm_encoder *drm_enc,
> >  	DPU_ATRACE_END("encoder_underrun_callback");
> >  }
> > 
> > -void dpu_encoder_register_vblank_callback(struct drm_encoder *drm_enc,
> > -		void (*vbl_cb)(void *), void *vbl_data)
> > +void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct
> > drm_crtc
> > *crtc)
> >  {
> >  	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> >  	unsigned long lock_flags;
> >  	bool enable;
> >  	int i;
> > 
> > -	enable = vbl_cb ? true : false;
> > +	enable = crtc ? true : false;
> > 
> >  	if (!drm_enc) {
> >  		DPU_ERROR("invalid encoder\n");
> > @@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct
> > drm_encoder *drm_enc,
> >  	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> > 
> >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > -	dpu_enc->crtc_vblank_cb = vbl_cb;
> > -	dpu_enc->crtc_vblank_cb_data = vbl_data;
> > +	/* crtc should always be cleared before re-assigning */
> > +	WARN_ON(crtc && dpu_enc->crtc);
> > +	dpu_enc->crtc = crtc;
> >  	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> > 
> >  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index aa4f135218fa..be1d80867834 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct drm_encoder
> > *encoder,
> >  				  struct dpu_encoder_hw_resources
> > *hw_res);
> > 
> >  /**
> > - * dpu_encoder_register_vblank_callback - provide callback to encoder
> > that
> > - *	will be called on the next vblank.
> > + * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned
> > to
> >   * @encoder:	encoder pointer
> > - * @cb:		callback pointer, provide NULL to deregister and
> > disable IRQs
> > - * @data:	user data provided to callback
> > + * @crtc:	crtc pointer
> >   */
> > -void dpu_encoder_register_vblank_callback(struct drm_encoder *encoder,
> > -		void (*cb)(void *), void *data);
> > +void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> > +			     struct drm_crtc *crtc);
> > 
> >  /**
> >   * dpu_encoder_register_frame_event_callback - provide callback to
> > encoder that
> 
> -- 
> Jeykumar S

-- 
Sean Paul, Software Engineer, Google / Chromium OS


More information about the dri-devel mailing list