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

["Ville Syrjälä" <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno