Re: [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 14/10/15 17:29, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird
> > rotation (to find the right GTT view for it), so no need to pass all
> > kinds of plane stuff.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++--------------------
> >   drivers/gpu/drm/i915/intel_drv.h     |  5 ++---
> >   drivers/gpu/drm/i915/intel_fbdev.c   |  2 +-
> >   3 files changed, 20 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 85e1473..80e9f2e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height,
> >   }
> >
> >   static int
> > -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> > -			const struct drm_plane_state *plane_state)
> > +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view,
> > +			const struct drm_framebuffer *fb,
> > +			unsigned int rotation)
> >   {
> >   	struct drm_i915_private *dev_priv = to_i915(fb->dev);
> >   	struct intel_rotation_info *info = &view->rotation_info;
> > @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> >
> >   	*view = i915_ggtt_view_normal;
> >
> > -	if (!plane_state)
> > -		return 0;
> > -
> > -	if (!intel_rotation_90_or_270(plane_state->rotation))
> > +	if (!intel_rotation_90_or_270(rotation))
> >   		return 0;
> >
> >   	*view = i915_ggtt_view_rotated;
> > @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct drm_i915_private *dev_priv
> >   }
> >
> >   int
> > -intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> > -			   struct drm_framebuffer *fb,
> > -			   const struct drm_plane_state *plane_state,
> > +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> > +			   unsigned int rotation,
> >   			   struct intel_engine_cs *pipelined,
> >   			   struct drm_i915_gem_request **pipelined_request)
> >   {
> 
> It feels like you are losing the benefit of cleaning this up by having 
> to pass in rotation anyway. So I think it makes more sense to keep 
> passing in plane_state and only get rid of the plane. Or vice-versa, not 
> really sure what is conceptually better. Possibly plane and then access 
> the state from it.

The only thing we basically need is "which vma do we want". But just
passing rotation directly looks nicer I think. The benefit really is
eliminating the ugly 'if (!plane_state)' mess caused by intel_fbdev.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx