Re: [PATCH 3/5] drm/i915: Clarify sprite plane function names (v3)

Daniel Vetter
2015-01-05T09:54:24Z

On Tue, Dec 23, 2014 at 12:08:29PM +0200, Ander Conselvan de Oliveira wrote:
> On 12/16/2014 02:23 AM, Matt Roper wrote:
> >A few of the sprite-related function names in i915 are very similar
> >(e.g., intel_enable_planes() vs intel_crtc_enable_planes()) and don't
> >make it clear whether they only operate on sprite planes, or whether
> >they also apply to all universal plane types. Rename a few functions to
> >be more consistent with our function naming for primary/cursor planes or
> >to clarify that they apply specifically to sprite planes:
> >
> > - s/intel_disable_planes/intel_disable_sprite_planes/
> > - s/intel_enable_planes/intel_enable_sprite_planes/
> >
> >Also, drop the sprite-specific intel_destroy_plane() and just use
> >the type-agnostic intel_plane_destroy() function. The extra 'disable'
> >call that intel_destroy_plane() did is unnecessary since the plane will
> >already be disabled due to framebuffer destruction by the point it gets
> >called.
> >
> >v2: Earlier consolidation patches have reduced the number of functions
> > we need to rename here.
> >
> >v3: Also rename intel_plane_funcs vtable to intel_sprite_plane_funcs
> > for consistency with primary/cursor. (Ander)
> >
> >Reviewed-by(v1): Bob Paauwe <bob.j.paauwe@intel.com>
> >Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >---
> > drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_sprite.c | 14 +++-----------
> > 3 files changed, 9 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index ce552d1..030cf93 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -4037,7 +4037,7 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
> > }
> > }
> >
> >-static void intel_enable_planes(struct drm_crtc *crtc)
> >+static void intel_enable_sprite_planes(struct drm_crtc *crtc)
> > {
> > struct drm_device *dev = crtc->dev;
> > enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >@@ -4051,7 +4051,7 @@ static void intel_enable_planes(struct drm_crtc *crtc)
> > }
> > }
> >
> >-static void intel_disable_planes(struct drm_crtc *crtc)
> >+static void intel_disable_sprite_planes(struct drm_crtc *crtc)
> > {
> > struct drm_device *dev = crtc->dev;
> > enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >@@ -4195,7 +4195,7 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> > int pipe = intel_crtc->pipe;
> >
> > intel_enable_primary_hw_plane(crtc->primary, crtc);
> >- intel_enable_planes(crtc);
> >+ intel_enable_sprite_planes(crtc);
> > intel_crtc_update_cursor(crtc, true);
> > intel_crtc_dpms_overlay(intel_crtc, true);
> >
> >@@ -4230,7 +4230,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
> >
> > intel_crtc_dpms_overlay(intel_crtc, false);
> > intel_crtc_update_cursor(crtc, false);
> >- intel_disable_planes(crtc);
> >+ intel_disable_sprite_planes(crtc);
> > intel_disable_primary_hw_plane(crtc->primary, crtc);
> >
> > /*
> >@@ -12008,7 +12008,7 @@ intel_disable_plane(struct drm_plane *plane)
> > }
> >
> > /* Common destruction function for both primary and cursor planes */
> >-static void intel_plane_destroy(struct drm_plane *plane)
> >+void intel_plane_destroy(struct drm_plane *plane)
>
> Do we need kernel doc for this?

Not sure kerneldoc for all the plane stuff is worth it yet while
everything is in-flux. Probably better to extract the higher-level stuff
into a new file/regroup functions a bit once the dust has settled and then
kerneldoc everything.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch