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. -- Ville Syrjälä Intel
