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

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 12:30:44PM +0100, Tvrtko Ursulin wrote:
> 
> On 15/10/15 12:17, Ville Syrjälä wrote:
> > On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote:
> >>
> >> Hi,
> >>
> >> On 14/10/15 17:29, ville.syrjala at linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala at 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 at 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.
> 
> We'll have to disagree there then because I find it really inelegant to 
> express the knowledge of what exact information is needed when preparing 
> the frame buffer for display into the function parameters.
> 
> It is conceptually much more elegant to say "this fb for this plane - do 
> what is right".

Only if the function is actually used for that. With fbdev it's not.
Chris did sell the "vma in plane state" idea to me last night on irc, so
when we get that we probably want a function that accepts the plane
state. But that can basically just wrap the current thing, which means
fbdev can keep using the lower level function that doesn't need the
plane state.

-- 
Ville Syrjälä
Intel OTC