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

From: Daniel Vetter <daniel@ffwll.ch>
To: ville.syrjala@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
Date: Wed, 21 Oct 2015 13:28:17 +0200	[thread overview]
Message-ID: <20151021112817.GC13786@phenom.ffwll.local> (raw)
In-Reply-To: <1444840154-7804-12-git-send-email-ville.syrjala@linux.intel.com>

On Wed, Oct 14, 2015 at 07:29:03PM +0300, 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>

Feels indeed a bit like a bikeshed and just churn without resolving the
"pass vma in plane_state around" idea for real. But meh, it's imo not
worse than the existing code, and looks correct. Less churn would make me
a happier reviewer thought (there's a pile of whitespace in here too).

Grumpily-Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  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)
>  {
> @@ -2371,7 +2368,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  
>  	alignment = intel_surf_alignment(dev_priv, fb->modifier[0]);
>  
> -	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
> +	ret = intel_fill_fb_ggtt_view(&view, fb, rotation);
>  	if (ret)
>  		return ret;
>  
> @@ -2432,8 +2429,7 @@ err_interruptible:
>  	return ret;
>  }
>  
> -static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
> -			       const struct drm_plane_state *plane_state)
> +static void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>  {
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct i915_ggtt_view view;
> @@ -2441,7 +2437,7 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
>  
>  	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
>  
> -	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
> +	ret = intel_fill_fb_ggtt_view(&view, fb, rotation);
>  	WARN_ONCE(ret, "Couldn't get view from plane state!");
>  
>  	i915_gem_object_unpin_fence(obj);
> @@ -10780,7 +10776,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>  	struct drm_plane *primary = crtc->base.primary;
>  
>  	mutex_lock(&dev->struct_mutex);
> -	intel_unpin_fb_obj(work->old_fb, primary->state);
> +	intel_unpin_fb_obj(work->old_fb, primary->state->rotation);
>  	drm_gem_object_unreference(&work->pending_flip_obj->base);
>  
>  	if (work->flip_queued_req)
> @@ -11521,8 +11517,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	 * synchronisation, so all we want here is to pin the framebuffer
>  	 * into the display plane and skip any waits.
>  	 */
> -	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb,
> -					 crtc->primary->state,
> +	ret = intel_pin_and_fence_fb_obj(fb, primary->state->rotation,
>  					 mmio_flip ? i915_gem_request_get_ring(obj->last_write_req) : ring, &request);
>  	if (ret)
>  		goto cleanup_pending;
> @@ -11573,7 +11568,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	return 0;
>  
>  cleanup_unpin:
> -	intel_unpin_fb_obj(fb, crtc->primary->state);
> +	intel_unpin_fb_obj(fb, crtc->primary->state->rotation);
>  cleanup_pending:
>  	if (request)
>  		i915_gem_request_cancel(request);
> @@ -13457,7 +13452,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  		if (ret)
>  			DRM_DEBUG_KMS("failed to attach phys object\n");
>  	} else {
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL);
> +		ret = intel_pin_and_fence_fb_obj(fb, new_state->rotation,
> +						 NULL, NULL);
>  	}
>  
>  	if (ret == 0)
> @@ -13488,7 +13484,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
>  	    !INTEL_INFO(dev)->cursor_needs_physical) {
>  		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_state->fb, old_state);
> +		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
>  		mutex_unlock(&dev->struct_mutex);
>  	}
>  }
> @@ -15474,9 +15470,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  			continue;
>  
>  		mutex_lock(&dev->struct_mutex);
> -		ret = intel_pin_and_fence_fb_obj(c->primary,
> -						 c->primary->fb,
> -						 c->primary->state,
> +		ret = intel_pin_and_fence_fb_obj(c->primary->fb,
> +						 c->primary->state->rotation,
>  						 NULL, NULL);
>  		mutex_unlock(&dev->struct_mutex);
>  		if (ret) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ed47ca3..b0d92e0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1067,9 +1067,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  void intel_release_load_detect_pipe(struct drm_connector *connector,
>  				    struct intel_load_detect_pipe *old,
>  				    struct drm_modeset_acquire_ctx *ctx);
> -int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
> -			       struct drm_framebuffer *fb,
> -			       const struct drm_plane_state *plane_state,
> +int 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);
>  struct drm_framebuffer *
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 4fd5fdf..6bef820 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -161,7 +161,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	}
>  
>  	/* Flush everything out, we'll be doing GTT only from now on */
> -	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
> +	ret = intel_pin_and_fence_fb_obj(fb, BIT(DRM_ROTATE_0), NULL, NULL);
>  	if (ret) {
>  		DRM_ERROR("failed to pin obj: %d\n", ret);
>  		goto out_fb;
> -- 
> 2.4.9
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx