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@xxxxxxxxxxxx>
> > >> >
> > >> > 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
