Re: [PATCH 2/7] drm/i915: Allow intel_plane_disable() to operate on all plane types

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Nov 13, 2014 at 09:23:02PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 13, 2014 at 11:11:38AM -0800, Bob Paauwe wrote:
> > On Thu, 13 Nov 2014 10:43:21 -0800
> > Matt Roper <matthew.d.roper@intel.com> wrote:
> > 
> > > We'll want to call this from the type-agnostic atomic plane helper
> > > hooks.  Since it's not sprite-specific anymore, more it to
> > > intel_display.c as well.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 10 +---------
> > >  3 files changed, 24 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index a9f90b8..c6598e9 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13679,3 +13679,24 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> > >  		spin_unlock_irq(&dev->event_lock);
> > >  	}
> > >  }
> > > +
> > > +void intel_plane_disable(struct drm_plane *plane)
> > > +{
> > > +	if (!plane->crtc || !plane->fb)
> > > +		return;
> > > +
> > > +	switch (plane->type) {
> > > +	case DRM_PLANE_TYPE_PRIMARY:
> > > +		intel_primary_plane_disable(plane);
> > > +		break;
> > > +	case DRM_PLANE_TYPE_CURSOR:
> > > +		intel_cursor_plane_disable(plane);
> > > +		break;
> > > +	case DRM_PLANE_TYPE_OVERLAY:
> > > +		intel_disable_plane(plane);
> > > +		break;
> > > +	default:
> > > +		WARN(1, "Unknown plane type");
> > > +	}
> > > +}
> > > +
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index bd5ef4e..df1420b 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -920,6 +920,7 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
> > >  void intel_finish_page_flip(struct drm_device *dev, int pipe);
> > >  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> > >  void intel_check_page_flip(struct drm_device *dev, int pipe);
> > > +void intel_plane_disable(struct drm_plane *plane);
> > >  
> > >  /* shared dpll functions */
> > >  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> > > @@ -1180,7 +1181,6 @@ int intel_plane_set_property(struct drm_plane *plane,
> > >  			     struct drm_property *prop,
> > >  			     uint64_t val);
> > >  int intel_plane_restore(struct drm_plane *plane);
> > > -void intel_plane_disable(struct drm_plane *plane);
> > >  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> > >  			      struct drm_file *file_priv);
> > >  int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> > > @@ -1188,6 +1188,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> > >  bool intel_pipe_update_start(struct intel_crtc *crtc,
> > >  			     uint32_t *start_vbl_count);
> > >  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> > > +int intel_disable_plane(struct drm_plane *plane);
> > 
> > Would it make sense to rename this to intel_sprite_plane_disable() as
> > part of this?  It would be more consistent with the cursor and primary
> > plane naming conventions and likely avoid some confusion with the
> > intel_plane_disable() function.
> 
> Hmm. Why do we even still have some kind of disable hook in the atomic
> age? I would expect to just have .commit() or somesuch thing.
> 
> But is there's still some need for a disable hook, shouldn't we just call
> the .disable_plane() hook of drm_plane? I guess I should really go and
> read some this new helper stuff...

You're right that it does eventually come down to
drm_plane_helper_commit() --> intel_plane_atomic_update() which should
be able to program the plane to either a new state or off.  To reuse as
much of our existing code as possible and keep my changes minimal, I'm
just implementing intel_plane_atomic_update() as:

        if (!plane->state->fb)
                intel_plane_disable(plane);
        else                                                                                                                                     
                intel_plane->commit_plane(plane, intel_state);                                                                                    

since we already had both of those functions implemented and working.
intel_plane->commit_plane() doesn't expect to be called in the disabled
case today (assumes crtc and/or fb are non-NULL), but it definitely
seems reasonable/easy to tweak that and roll the disable logic directly
into the lowest-level commit_plane in the future.  I just wanted to keep
things as simple as possible with minimal code changes for the initial
patchset.


Matt

> 
> > >  
> > >  /* intel_tv.c */
> > >  void intel_tv_init(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index fc96d13..115acd3 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1425,7 +1425,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  	return 0;
> > >  }
> > >  
> > > -static int
> > > +int
> > >  intel_disable_plane(struct drm_plane *plane)
> > >  {
> > >  	struct drm_device *dev = plane->dev;
> > > @@ -1576,14 +1576,6 @@ int intel_plane_restore(struct drm_plane *plane)
> > >  				  intel_plane->src_w, intel_plane->src_h);
> > >  }
> > >  
> > > -void intel_plane_disable(struct drm_plane *plane)
> > > -{
> > > -	if (!plane->crtc || !plane->fb)
> > > -		return;
> > > -
> > > -	intel_disable_plane(plane);
> > > -}
> > > -
> > >  static const struct drm_plane_funcs intel_plane_funcs = {
> > >  	.update_plane = intel_update_plane,
> > >  	.disable_plane = intel_disable_plane,
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel