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

[Jeykumar Sankaran]

On 2018-11-14 12:52, Sean Paul wrote:
> 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
> 
That explains it! Thanks 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

-- 
Jeykumar S