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