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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 13, 2014 at 04:35:38AM +0000, G, Pallavi wrote:
> 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>

Thanks a lot for the review and patches, all merged to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch