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

[Sean Paul]

On Wed, Nov 14, 2018 at 12:46:32PM -0800, Jeykumar Sankaran wrote:
> On 2018-11-14 11:57, Ville Syrjälä wrote:
> > 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 at 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.
> > 
> > drm_vblank_off()
> > // .enable_vblank/.disable_vblank will never be called here
> > drm_vblank_on()
> > 
> > So if you use drm_vblank_on/off judiciously you will never
> > have to deal with this particular problem.
> > 
> Thanks ville for clarifying that.
> 
> We can make sure to avoid that sequence if we have control over it. In DPU,
> CRTC calls
> dpu_vblank_off in crtc_disable. After disabling, no one stopping
> userspace clients to call into DRM_IOCTL_WAIT_VBLANK on the crtc. This ioctl
> calls enable_vblank when it sees the vblank is disabled [1].


That's prevented by this bit in drm_vblank.c:

	/*
	 * Prevent subsequent drm_vblank_get() from re-enabling
	 * the vblank interrupt by bumping the refcount.
	 */
	if (!vblank->inmodeset) {
		atomic_inc(&vblank->refcount);
		vblank->inmodeset = 1;
	}

Basically it turns off the hw and then takes a reference such that
drm_vblank_get will never call drm_vblank_enable.

Sean

> 
> So far, the dpu_crtc_vblank was failing when it finds that the crtc is
> disabled.
> With this patch, the condition was removed, so I was wondering what is the
> expected behavior with this patch.
> 
> [1] https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/drivers/gpu/drm/drm_vblank.c#L956
> 
> Thanks and Regards,
> Jeykumar S.
> > --
> > Ville Syrjälä
> > Intel
> 
> -- 
> Jeykumar S

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