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

[Jeykumar Sankaran]

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].

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