Re: [PATCH 6/8] drm/msm: dpu: Separate crtc assignment from vblank enable

[Sean Paul <sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>]

On Wed, Nov 14, 2018 at 11:43:51AM -0800, Jeykumar Sankaran wrote:
> On 2018-11-14 07:16, Sean Paul wrote:
> > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote:
> > > On 2018-11-13 12:52, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > Instead of assigning/clearing the crtc on vblank enable/disable, we
> > can
> > > > just assign and clear the crtc on modeset. That allows us to just
> > toggle
> > > > the encoder's vblank interrupts on vblank_enable.
> > > >
> > > > So why is this important? Previously the driver was using the legacy
> > > > pointers to assign/clear the crtc. Legacy pointers are cleared _after_
> > > Which pointers are you referring here as legacy pointers? CRTC?
> > 
> > encoder->crtc in this case. The loop in vblank_enable_no_lock relies on
> > enc->crtc == crtc
> > 
> > > > disabling the hardware, so the legacy pointer was valid during
> > > > vblank_disable, but that's not something we should rely on.
> > > >
> > > > Instead of relying on the core ordering the legacy pointer assignments
> > > > just so, we'll assign the crtc in dpu_crtc enable/disable. This is the
> > > > only place that mapping can change, so we're covered there.
> > > >
> > > > We're also taking advantage of drm_crtc_vblank_on/off. By using this,
> > we
> > > > ensure that vblank_enable/disable can never be called while the crtc
> > is
> > > > off (which means the assigned crtc will always be valid). As such, we
> > > 
> > > What about the drm_vblank_enable/disable triggered by drm_vblank_get
> > when
> > > crtc
> > > is disabled? What is the expected behavior there? the vblank_requested
> > > variable removed by the next patch was introduced to cache the
> > > request.
> > 
> > That case is covered by the modeset locks held when calling disable().
> > 
> I am sure it will take care of drm_crtc_vblank_on/off triggered within
> crtc_disable.
> My question was what was the expected behaviour when DRM_IOCTL_WAIT_VBLANK
> is
> called when crtc is disabled? the ioctl will try to call drm_vblank_get and
> I
> don't see the patch checking for crtc being enabled in the path.

It's the same as any other drm_vblank_get call when the crtc is disabled, it's
gated internally by the core when vblank is turned off.

> 
> Thanks and Regards,
> Jeykumar S.
> 
> 
> > > 
> > > > don't need to use modeset locks or the crtc_lock in the
> > > > vblank_enable/disable routine to be sure state is consistent.
> > > >
> > > > ...I think.
> > > >
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 77
> > +++++++++------------
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++---
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 +++
> > > >  3 files changed, 59 insertions(+), 55 deletions(-)
> > > >

/snip

> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > index fd6514f681ae..5914ae70572c 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > @@ -1267,22 +1267,29 @@ 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 = crtc ? true : false;
> > > > -
> > > > -	if (!drm_enc) {
> > > > -		DPU_ERROR("invalid encoder\n");
> > > > -		return;
> > > > -	}
> > > > -	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> > > >
> > > >  	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > > >  	/* 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);
> > > > +}
> > > > +
> > > > +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
> > > > +					struct drm_crtc *crtc, bool
> > > > enable)
> > > > +{
> > > > +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > > +	unsigned long lock_flags;
> > > > +	int i;
> > > > +
> > > > +	trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> > > > +
> > > > +	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > > > +	if (dpu_enc->crtc == crtc) {
> > > > +		spin_unlock_irqrestore(&dpu_enc->enc_spinlock,
> > > > lock_flags);
> > > > +		return;
> > > > +	}
> > > Why are you returning when the crtc's are same?
> > > Won't they be same all the time between modesets?
> > > 

Ugh, well that's embarrassing! This is supposed to be !=

> > > for both enable/disable crtc will be the same and you still need to
> > > call
> > the
> > > for loop below to enable/disable the phys encoders.
> > > 
> 
> Can you go through my comments above?

Shoot, sorry, I didn't see these.

Sean

> 
> > > > +	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> > > >
> > > >  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > > >  		struct dpu_encoder_phys *phys = dpu_enc->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 be1d80867834..6896ea2ab854 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > @@ -62,6 +62,16 @@ void dpu_encoder_get_hw_resources(struct
> > drm_encoder
> > > > *encoder,
> > > >  void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> > > >  			     struct drm_crtc *crtc);
> > > >
> > > > +/**
> > > > + * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on
> > or
> > > > off if
> > > > + *	the encoder is assigned to the given crtc
> > > > + * @encoder:	encoder pointer
> > > > + * @crtc:	crtc pointer
> > > > + * @enable:	true if vblank should be enabled
> > > > + */
> > > > +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *encoder,
> > > > +					struct drm_crtc *crtc, bool
> > > > enable);
> > > > +
> > > >  /**
> > > >   * dpu_encoder_register_frame_event_callback - provide callback to
> > > > encoder that
> > > >   *	will be called after the request is complete, or other events.
> > > 
> > > --
> > > Jeykumar S
> 
> -- 
> Jeykumar S

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno