Re: [PATCH v3 05/11] drm/i915: remove intel_crtc_cursor_set_obj()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 24, 2014 at 02:23:35PM +0100, Gustavo Padovan wrote:
> 2014-10-07 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
> 
> > On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > > 
> > > Merge it into the plane update_plane() callback and make other
> > > users use the update_plane() functions instead.
> > > 
> > > The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
> > > so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
> > > and merge both paths into one.
> > > 
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 221 +++++++++++++++++------------------
> > >  1 file changed, 106 insertions(+), 115 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f91a5b0..a583abd 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev,
> > >  	return true;
> > >  }
> > >  
> > > -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> > > -				     struct drm_i915_gem_object *obj,
> > > -				     uint32_t width, uint32_t height)
> > > -{
> > > -	struct drm_device *dev = crtc->dev;
> > > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > -	enum pipe pipe = intel_crtc->pipe;
> > > -	unsigned old_width;
> > > -	uint32_t addr;
> > > -	int ret;
> > > -
> > > -	/* if we want to turn off the cursor ignore width and height */
> > > -	if (!obj) {
> > > -		DRM_DEBUG_KMS("cursor off\n");
> > > -		addr = 0;
> > > -		mutex_lock(&dev->struct_mutex);
> > > -		goto finish;
> > > -	}
> > > -
> > > -	/* we only need to pin inside GTT if cursor is non-phy */
> > > -	mutex_lock(&dev->struct_mutex);
> > > -	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> > > -		unsigned alignment;
> > > -
> > > -		/*
> > > -		 * Global gtt pte registers are special registers which actually
> > > -		 * forward writes to a chunk of system memory. Which means that
> > > -		 * there is no risk that the register values disappear as soon
> > > -		 * as we call intel_runtime_pm_put(), so it is correct to wrap
> > > -		 * only the pin/unpin/fence and not more.
> > > -		 */
> > > -		intel_runtime_pm_get(dev_priv);
> > > -
> > > -		/* Note that the w/a also requires 2 PTE of padding following
> > > -		 * the bo. We currently fill all unused PTE with the shadow
> > > -		 * page and so we should always have valid PTE following the
> > > -		 * cursor preventing the VT-d warning.
> > > -		 */
> > > -		alignment = 0;
> > > -		if (need_vtd_wa(dev))
> > > -			alignment = 64*1024;
> > > -
> > > -		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> > > -		if (ret) {
> > > -			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> > > -			intel_runtime_pm_put(dev_priv);
> > > -			goto fail_locked;
> > > -		}
> > > -
> > > -		ret = i915_gem_object_put_fence(obj);
> > > -		if (ret) {
> > > -			DRM_DEBUG_KMS("failed to release fence for cursor");
> > > -			intel_runtime_pm_put(dev_priv);
> > > -			goto fail_unpin;
> > > -		}
> > > -
> > > -		addr = i915_gem_obj_ggtt_offset(obj);
> > > -
> > > -		intel_runtime_pm_put(dev_priv);
> > > -	} else {
> > > -		int align = IS_I830(dev) ? 16 * 1024 : 256;
> > > -		ret = i915_gem_object_attach_phys(obj, align);
> > > -		if (ret) {
> > > -			DRM_DEBUG_KMS("failed to attach phys object\n");
> > > -			goto fail_locked;
> > > -		}
> > > -		addr = obj->phys_handle->busaddr;
> > > -	}
> > > -
> > > - finish:
> > > -	if (intel_crtc->cursor_bo) {
> > > -		if (!INTEL_INFO(dev)->cursor_needs_physical)
> > > -			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> > > -	}
> > > -
> > > -	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> > > -			  INTEL_FRONTBUFFER_CURSOR(pipe));
> > > -	mutex_unlock(&dev->struct_mutex);
> > > -
> > > -	old_width = intel_crtc->cursor_width;
> > > -
> > > -	intel_crtc->cursor_addr = addr;
> > > -	intel_crtc->cursor_bo = obj;
> > > -	intel_crtc->cursor_width = width;
> > > -	intel_crtc->cursor_height = height;
> > > -
> > > -	if (intel_crtc->active) {
> > > -		if (old_width != width)
> > > -			intel_update_watermarks(crtc);
> > > -		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> > > -	}
> > > -
> > > -	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> > > -
> > > -	return 0;
> > > -fail_unpin:
> > > -	i915_gem_object_unpin_from_display_plane(obj);
> > > -fail_locked:
> > > -	mutex_unlock(&dev->struct_mutex);
> > > -	return ret;
> > > -}
> > > -
> > >  static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
> > >  				 u16 *blue, uint32_t start, uint32_t size)
> > >  {
> > > @@ -11897,7 +11794,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
> > >  
> > >  	BUG_ON(!plane->crtc);
> > >  
> > > -	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> > > +	return plane->funcs->update_plane(plane, plane->crtc, NULL,
> > > +					  0, 0, 0, 0, 0, 0, 0, 0);
> > >  }
> > >  
> > >  static int
> > > @@ -11961,26 +11859,119 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> > >  			  struct intel_plane_state *state)
> > >  {
> > >  	struct drm_crtc *crtc = state->crtc;
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct drm_framebuffer *fb = state->fb;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > -	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > > -	struct drm_i915_gem_object *obj = intel_fb->obj;
> > > -	int crtc_w, crtc_h;
> > > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > +	enum pipe pipe = intel_crtc->pipe;
> > > +	unsigned old_width;
> > > +	uint32_t addr;
> > > +	bool on;
> > > +	int ret;
> > >  
> > >  	crtc->cursor_x = state->orig_dst.x1;
> > >  	crtc->cursor_y = state->orig_dst.y1;
> > > -	if (fb != crtc->cursor->fb) {
> > > -		crtc_w = drm_rect_width(&state->orig_dst);
> > > -		crtc_h = drm_rect_height(&state->orig_dst);
> > > -		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> > > +
> > > +	if (intel_crtc->cursor_bo == obj)
> > > +		goto update;
> > > +
> > > +	/* if we want to turn off the cursor ignore width and height */
> > > +	if (!obj) {
> > > +		DRM_DEBUG_KMS("cursor off\n");
> > > +		addr = 0;
> > > +		mutex_lock(&dev->struct_mutex);
> > > +		goto finish;
> > > +	}
> > > +
> > > +	/* we only need to pin inside GTT if cursor is non-phy */
> > > +	mutex_lock(&dev->struct_mutex);
> > > +	if (!INTEL_INFO(dev)->cursor_needs_physical) {
> > > +		unsigned alignment;
> > > +
> > > +		/*
> > > +		 * Global gtt pte registers are special registers which actually
> > > +		 * forward writes to a chunk of system memory. Which means that
> > > +		 * there is no risk that the register values disappear as soon
> > > +		 * as we call intel_runtime_pm_put(), so it is correct to wrap
> > > +		 * only the pin/unpin/fence and not more.
> > > +		 */
> > > +		intel_runtime_pm_get(dev_priv);
> > > +
> > > +		/* Note that the w/a also requires 2 PTE of padding following
> > > +		 * the bo. We currently fill all unused PTE with the shadow
> > > +		 * page and so we should always have valid PTE following the
> > > +		 * cursor preventing the VT-d warning.
> > > +		 */
> > > +		alignment = 0;
> > > +		if (need_vtd_wa(dev))
> > > +			alignment = 64*1024;
> > > +
> > > +		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> > > +		if (ret) {
> > > +			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> > > +			intel_runtime_pm_put(dev_priv);
> > > +			goto fail_locked;
> > > +		}
> > > +
> > > +		ret = i915_gem_object_put_fence(obj);
> > > +		if (ret) {
> > > +			DRM_DEBUG_KMS("failed to release fence for cursor");
> > > +			intel_runtime_pm_put(dev_priv);
> > > +			goto fail_unpin;
> > > +		}
> > > +
> > > +		addr = i915_gem_obj_ggtt_offset(obj);
> > > +
> > > +		intel_runtime_pm_put(dev_priv);
> > >  	} else {
> > > -		intel_crtc_update_cursor(crtc, state->visible);
> > > +		int align = IS_I830(dev) ? 16 * 1024 : 256;
> > > +		ret = i915_gem_object_attach_phys(obj, align);
> > > +		if (ret) {
> > > +			DRM_DEBUG_KMS("failed to attach phys object\n");
> > > +			goto fail_locked;
> > > +		}
> > > +		addr = obj->phys_handle->busaddr;
> > > +	}
> > >  
> > > -		intel_frontbuffer_flip(crtc->dev,
> > > -				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> > > +finish:
> > > +	if (intel_crtc->cursor_bo) {
> > > +		if (!INTEL_INFO(dev)->cursor_needs_physical)
> > > +			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> > > +	}
> > >  
> > > -		return 0;
> > > +	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> > > +			  INTEL_FRONTBUFFER_CURSOR(pipe));
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +
> > > +	intel_crtc->cursor_addr = addr;
> > > +	intel_crtc->cursor_bo = obj;
> > > +update:
> > > +	old_width = intel_crtc->cursor_width;
> > > +
> > > +	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> > > +	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> > > +
> > > +	if (intel_crtc->cursor_bo == obj)
> > > +		on = state->visible;
> > > +	else
> > > +		on = !obj;
> > 
> > That looks fishy. Why do we need to care if the bo changed here, and why
> > would we turn on the cursor when there's no bo?
> 
> Yes, this should actually have been on = !!obj. But all I was doing here is
> reordering the code, so this is how it was before. The flow is the same.
> 
> > 
> > The cursor is either visible or it isn't, and that's all that
> > intel_crtc_update_cursor() should care about.
> 
> Right, I'll remove the if-else and call intel_crtc_update_cursor() with
> state->visible as parameter.

I just realized that we don't seem to actually compute state->visible
correctly when there's no fb. So that definitely needs to be done if we
want to share the .update_plane() as .disable_plane().

> 
> > 
> > > +
> > > +	if (intel_crtc->active) {
> > > +		if (old_width != intel_crtc->cursor_width)
> > > +			intel_update_watermarks(crtc);
> > > +		intel_crtc_update_cursor(crtc, on);
> > >  	}
> > > +
> > > +	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> > 
> > This should probably be done only when crtc->active==true. I see we're
> > not doing it that way currently, so I guess we could have a separate
> > patch to change that.
> 
> Okay, I have created a separated patch for this.
> 
> 	Gustavo

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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux