Re: [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4)

["G, Pallavi" <pallavi.g@intel.com>]

On Thu, 2014-06-12 at 07:57 -0700, Matt Roper wrote:
> On Thu, Jun 12, 2014 at 04:47:18AM -0700, G, Pallavi wrote:
> > On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> ...
> > > @@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
> > >  		kfree(work);
> > >  	}
> > >  
> > > -	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0)
> > 
> >         Please help me to understand how the cursor enable/disable will
> > handled in the legacy path if we remove the cursor disable from
> > intel_crtc_destroy
> 
> Good question.  When the driver is shutting down the DRM core tears down
> all the KMS stuff associated with the driver.  One of those steps is
> destroying the CRTC which, as you note, previously would take care of
> turning off the cursor plane.  However now that the cursor exists as its
> own drm_plane, and whatever is being scanned out by the cursor is a real
> drm_framebuffer that the DRM core knows about, the cursor's FB should
> get destroyed by the DRM core, which will trigger the cursor disable
> entrypoint on the driver.  The call sequence is
> 
> drm_framebuffer_remove() -> drm_force_plane_disable()
>                          -> intel_cursor_plane_disable()
> 
> 
> > > -
> > >  	drm_crtc_cleanup(crtc);
> > >  
> > >  	kfree(intel_crtc);
> > > @@ -10942,8 +10912,6 @@ out_config:
> > >  }
> > >  
> > >  static const struct drm_crtc_funcs intel_crtc_funcs = {
> > > -	.cursor_set = intel_crtc_cursor_set,
> > > -	.cursor_move = intel_crtc_cursor_move,
> > 
> > I don't find the corresponding changes in the drm layer related to the
> > cursor_set and cursor_move removal. Even in the patch 3 only for the
> > universal plane drm_mode_cursor_universal is called what about the
> > legacy path?
> 
> Right, the driver entrypoints remain in the drm_crtc_funcs structure
> because other non-i915 drivers may not have implemented universal cursor
> planes, so the DRM core will still call into their .cursor_set and
> .cursor_move entrypoints when they issue a legacy cursor ioctl.  We
> don't want to force other driver authors to update to universal planes
> until they're ready, so we want to keep the old code paths working until
> everyone has updated.
> 
> Drivers like i915 that do get updated to have universal cursor planes
> will no longer receive calls into these legacy entrypoints anymore
> (since everything will come into the universal entrypoint), so there's
> no need for us to keep the legacy .cursor_set and .cursor_move around in
> our own driver.
> 
> 
> Matt
> 
Thanks for the clarification.
with this my doubts are clear. I reviewed the entire patch series with
respect to the legacy support as well as the universal cursor plane
support in general. I dont find any issues and design gaps. I hope Matt
is working on the igt based test for the universal plane support (seen
some patches related to that in the mail thread).

Reviewed-by: Pallavi G <pallavi.g@intel.com>