Re: [PATCH 1/2] drm/i915/overlay: Allocate physical registers from stolen

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

On Thu, Sep 06, 2018 at 08:01:43PM +0100, Chris Wilson wrote:
> Given that we are now reasonably confident in our ability to detect and
> reserve the stolen memory (physical memory reserved for graphics by the
> BIOS) for ourselves on most machines, we can put it to use. In this
> case, we need a page to hold the overlay registers.
> 
> On an i915g running MythTv, H Buus noticed that
> 
> 	commit 6a2c4232ece145d8b5a8f95f767bd6d0d2d2f2bb
> 	Author: Chris Wilson <chris@chris-wilson.co.uk>
> 	Date:   Tue Nov 4 04:51:40 2014 -0800
> 	drm/i915: Make the physical object coherent with GTT
> 
> introduced stuttering into his video playback. After discarding the
> likely suspect of it being the physical cursor updates, we were left
> with the use of the phys object for the overlay. And lo, if we
> completely avoid using the phys object (allocated just once on module
> load!) by switching to stolen memory, the stuttering goes away.
> 
> For lack of a better explanation, claim victory and kill two birds with
> one stone.

A but peculiar. But looks fine to me. And we should have some
stolen pretty much everywhere. I guess my only concern is permanently
fragmenting stolen with this. Or does the allocator grab it from the
very end?

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107600
> Fixes: 6a2c4232ece1 ("drm/i915: Make the physical object coherent with GTT")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_overlay.c | 228 +++++++++------------------
>  1 file changed, 75 insertions(+), 153 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index c2f10d899329..443dfaefd7a6 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -181,8 +181,9 @@ struct intel_overlay {
>  	u32 brightness, contrast, saturation;
>  	u32 old_xscale, old_yscale;
>  	/* register access */
> -	u32 flip_addr;
>  	struct drm_i915_gem_object *reg_bo;
> +	struct overlay_registers __iomem *regs;
> +	u32 flip_addr;
>  	/* flip handling */
>  	struct i915_gem_active last_flip;
>  };
> @@ -210,29 +211,6 @@ static void i830_overlay_clock_gating(struct drm_i915_private *dev_priv,
>  				  PCI_DEVFN(0, 0), I830_CLOCK_GATE, val);
>  }
>  
> -static struct overlay_registers __iomem *
> -intel_overlay_map_regs(struct intel_overlay *overlay)
> -{
> -	struct drm_i915_private *dev_priv = overlay->i915;
> -	struct overlay_registers __iomem *regs;
> -
> -	if (OVERLAY_NEEDS_PHYSICAL(dev_priv))
> -		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
> -	else
> -		regs = io_mapping_map_wc(&dev_priv->ggtt.iomap,
> -					 overlay->flip_addr,
> -					 PAGE_SIZE);
> -
> -	return regs;
> -}
> -
> -static void intel_overlay_unmap_regs(struct intel_overlay *overlay,
> -				     struct overlay_registers __iomem *regs)
> -{
> -	if (!OVERLAY_NEEDS_PHYSICAL(overlay->i915))
> -		io_mapping_unmap(regs);
> -}
> -
>  static void intel_overlay_submit_request(struct intel_overlay *overlay,
>  					 struct i915_request *rq,
>  					 i915_gem_retire_fn retire)
> @@ -784,13 +762,13 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  				      struct drm_i915_gem_object *new_bo,
>  				      struct put_image_params *params)
>  {
> -	int ret, tmp_width;
> -	struct overlay_registers __iomem *regs;
> -	bool scale_changed = false;
> +	struct overlay_registers __iomem *regs = overlay->regs;
>  	struct drm_i915_private *dev_priv = overlay->i915;
>  	u32 swidth, swidthsw, sheight, ostride;
>  	enum pipe pipe = overlay->crtc->pipe;
> +	bool scale_changed = false;
>  	struct i915_vma *vma;
> +	int ret, tmp_width;
>  
>  	lockdep_assert_held(&dev_priv->drm.struct_mutex);
>  	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
> @@ -815,30 +793,19 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  
>  	if (!overlay->active) {
>  		u32 oconfig;
> -		regs = intel_overlay_map_regs(overlay);
> -		if (!regs) {
> -			ret = -ENOMEM;
> -			goto out_unpin;
> -		}
> +
>  		oconfig = OCONF_CC_OUT_8BIT;
>  		if (IS_GEN4(dev_priv))
>  			oconfig |= OCONF_CSC_MODE_BT709;
>  		oconfig |= pipe == 0 ?
>  			OCONF_PIPE_A : OCONF_PIPE_B;
>  		iowrite32(oconfig, &regs->OCONFIG);
> -		intel_overlay_unmap_regs(overlay, regs);
>  
>  		ret = intel_overlay_on(overlay);
>  		if (ret != 0)
>  			goto out_unpin;
>  	}
>  
> -	regs = intel_overlay_map_regs(overlay);
> -	if (!regs) {
> -		ret = -ENOMEM;
> -		goto out_unpin;
> -	}
> -
>  	iowrite32((params->dst_y << 16) | params->dst_x, &regs->DWINPOS);
>  	iowrite32((params->dst_h << 16) | params->dst_w, &regs->DWINSZ);
>  
> @@ -882,8 +849,6 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  
>  	iowrite32(overlay_cmd_reg(params), &regs->OCMD);
>  
> -	intel_overlay_unmap_regs(overlay, regs);
> -
>  	ret = intel_overlay_continue(overlay, vma, scale_changed);
>  	if (ret)
>  		goto out_unpin;
> @@ -901,7 +866,6 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  int intel_overlay_switch_off(struct intel_overlay *overlay)
>  {
>  	struct drm_i915_private *dev_priv = overlay->i915;
> -	struct overlay_registers __iomem *regs;
>  	int ret;
>  
>  	lockdep_assert_held(&dev_priv->drm.struct_mutex);
> @@ -918,9 +882,7 @@ int intel_overlay_switch_off(struct intel_overlay *overlay)
>  	if (ret != 0)
>  		return ret;
>  
> -	regs = intel_overlay_map_regs(overlay);
> -	iowrite32(0, &regs->OCMD);
> -	intel_overlay_unmap_regs(overlay, regs);
> +	iowrite32(0, &overlay->regs->OCMD);
>  
>  	return intel_overlay_off(overlay);
>  }
> @@ -1305,7 +1267,6 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data,
>  	struct drm_intel_overlay_attrs *attrs = data;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_overlay *overlay;
> -	struct overlay_registers __iomem *regs;
>  	int ret;
>  
>  	overlay = dev_priv->overlay;
> @@ -1345,15 +1306,7 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data,
>  		overlay->contrast   = attrs->contrast;
>  		overlay->saturation = attrs->saturation;
>  
> -		regs = intel_overlay_map_regs(overlay);
> -		if (!regs) {
> -			ret = -ENOMEM;
> -			goto out_unlock;
> -		}
> -
> -		update_reg_attrs(overlay, regs);
> -
> -		intel_overlay_unmap_regs(overlay, regs);
> +		update_reg_attrs(overlay, overlay->regs);
>  
>  		if (attrs->flags & I915_OVERLAY_UPDATE_GAMMA) {
>  			if (IS_GEN2(dev_priv))
> @@ -1386,12 +1339,47 @@ int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data,
>  	return ret;
>  }
>  
> +static int get_registers(struct intel_overlay *overlay, bool use_phys)
> +{
> +	struct drm_i915_gem_object *obj;
> +	struct i915_vma *vma;
> +	int err;
> +
> +	obj = i915_gem_object_create_stolen(overlay->i915, PAGE_SIZE);
> +	if (obj == NULL)
> +		obj = i915_gem_object_create_internal(overlay->i915, PAGE_SIZE);
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
> +
> +	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE);
> +	if (IS_ERR(vma)) {
> +		err = PTR_ERR(vma);
> +		goto err_put_bo;
> +	}
> +
> +	if (use_phys)
> +		overlay->flip_addr = sg_dma_address(obj->mm.pages->sgl);
> +	else
> +		overlay->flip_addr = i915_ggtt_offset(vma);
> +	overlay->regs = i915_vma_pin_iomap(vma);
> +	i915_vma_unpin(vma);
> +
> +	if (IS_ERR(overlay->regs)) {
> +		err = PTR_ERR(overlay->regs);
> +		goto err_put_bo;
> +	}
> +
> +	overlay->reg_bo = obj;
> +	return 0;
> +
> +err_put_bo:
> +	i915_gem_object_put(obj);
> +	return err;
> +}
> +
>  void intel_setup_overlay(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_overlay *overlay;
> -	struct drm_i915_gem_object *reg_bo;
> -	struct overlay_registers __iomem *regs;
> -	struct i915_vma *vma = NULL;
>  	int ret;
>  
>  	if (!HAS_OVERLAY(dev_priv))
> @@ -1401,46 +1389,8 @@ void intel_setup_overlay(struct drm_i915_private *dev_priv)
>  	if (!overlay)
>  		return;
>  
> -	mutex_lock(&dev_priv->drm.struct_mutex);
> -	if (WARN_ON(dev_priv->overlay))
> -		goto out_free;
> -
>  	overlay->i915 = dev_priv;
>  
> -	reg_bo = NULL;
> -	if (!OVERLAY_NEEDS_PHYSICAL(dev_priv))
> -		reg_bo = i915_gem_object_create_stolen(dev_priv, PAGE_SIZE);
> -	if (reg_bo == NULL)
> -		reg_bo = i915_gem_object_create(dev_priv, PAGE_SIZE);
> -	if (IS_ERR(reg_bo))
> -		goto out_free;
> -	overlay->reg_bo = reg_bo;
> -
> -	if (OVERLAY_NEEDS_PHYSICAL(dev_priv)) {
> -		ret = i915_gem_object_attach_phys(reg_bo, PAGE_SIZE);
> -		if (ret) {
> -			DRM_ERROR("failed to attach phys overlay regs\n");
> -			goto out_free_bo;
> -		}
> -		overlay->flip_addr = reg_bo->phys_handle->busaddr;
> -	} else {
> -		vma = i915_gem_object_ggtt_pin(reg_bo, NULL,
> -					       0, PAGE_SIZE, PIN_MAPPABLE);
> -		if (IS_ERR(vma)) {
> -			DRM_ERROR("failed to pin overlay register bo\n");
> -			ret = PTR_ERR(vma);
> -			goto out_free_bo;
> -		}
> -		overlay->flip_addr = i915_ggtt_offset(vma);
> -
> -		ret = i915_gem_object_set_to_gtt_domain(reg_bo, true);
> -		if (ret) {
> -			DRM_ERROR("failed to move overlay register bo into the GTT\n");
> -			goto out_unpin_bo;
> -		}
> -	}
> -
> -	/* init all values */
>  	overlay->color_key = 0x0101fe;
>  	overlay->color_key_enabled = true;
>  	overlay->brightness = -19;
> @@ -1449,44 +1399,51 @@ void intel_setup_overlay(struct drm_i915_private *dev_priv)
>  
>  	init_request_active(&overlay->last_flip, NULL);
>  
> -	regs = intel_overlay_map_regs(overlay);
> -	if (!regs)
> -		goto out_unpin_bo;
> +	mutex_lock(&dev_priv->drm.struct_mutex);
> +
> +	ret = get_registers(overlay, OVERLAY_NEEDS_PHYSICAL(dev_priv));
> +	if (ret)
> +		goto out_free;
> +
> +	ret = i915_gem_object_set_to_gtt_domain(overlay->reg_bo, true);
> +	if (ret)
> +		goto out_reg_bo;
>  
> -	memset_io(regs, 0, sizeof(struct overlay_registers));
> -	update_polyphase_filter(regs);
> -	update_reg_attrs(overlay, regs);
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
>  
> -	intel_overlay_unmap_regs(overlay, regs);
> +	memset_io(overlay->regs, 0, sizeof(struct overlay_registers));
> +	update_polyphase_filter(overlay->regs);
> +	update_reg_attrs(overlay, overlay->regs);
>  
>  	dev_priv->overlay = overlay;
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> -	DRM_INFO("initialized overlay support\n");
> +	DRM_INFO("Initialized overlay support.\n");
>  	return;
>  
> -out_unpin_bo:
> -	if (vma)
> -		i915_vma_unpin(vma);
> -out_free_bo:
> -	i915_gem_object_put(reg_bo);
> +out_reg_bo:
> +	i915_gem_object_put(overlay->reg_bo);
>  out_free:
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  	kfree(overlay);
> -	return;
>  }
>  
>  void intel_cleanup_overlay(struct drm_i915_private *dev_priv)
>  {
> -	if (!dev_priv->overlay)
> +	struct intel_overlay *overlay;
> +
> +	overlay = fetch_and_zero(&dev_priv->overlay);
> +	if (!overlay)
>  		return;
>  
> -	/* The bo's should be free'd by the generic code already.
> +	/*
> +	 * The bo's should be free'd by the generic code already.
>  	 * Furthermore modesetting teardown happens beforehand so the
> -	 * hardware should be off already */
> -	WARN_ON(dev_priv->overlay->active);
> +	 * hardware should be off already.
> +	 */
> +	WARN_ON(overlay->active);
> +
> +	i915_gem_object_put(overlay->reg_bo);
>  
> -	i915_gem_object_put(dev_priv->overlay->reg_bo);
> -	kfree(dev_priv->overlay);
> +	kfree(overlay);
>  }
>  
>  #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
> @@ -1498,37 +1455,11 @@ struct intel_overlay_error_state {
>  	u32 isr;
>  };
>  
> -static struct overlay_registers __iomem *
> -intel_overlay_map_regs_atomic(struct intel_overlay *overlay)
> -{
> -	struct drm_i915_private *dev_priv = overlay->i915;
> -	struct overlay_registers __iomem *regs;
> -
> -	if (OVERLAY_NEEDS_PHYSICAL(dev_priv))
> -		/* Cast to make sparse happy, but it's wc memory anyway, so
> -		 * equivalent to the wc io mapping on X86. */
> -		regs = (struct overlay_registers __iomem *)
> -			overlay->reg_bo->phys_handle->vaddr;
> -	else
> -		regs = io_mapping_map_atomic_wc(&dev_priv->ggtt.iomap,
> -						overlay->flip_addr);
> -
> -	return regs;
> -}
> -
> -static void intel_overlay_unmap_regs_atomic(struct intel_overlay *overlay,
> -					struct overlay_registers __iomem *regs)
> -{
> -	if (!OVERLAY_NEEDS_PHYSICAL(overlay->i915))
> -		io_mapping_unmap_atomic(regs);
> -}
> -
>  struct intel_overlay_error_state *
>  intel_overlay_capture_error_state(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_overlay *overlay = dev_priv->overlay;
>  	struct intel_overlay_error_state *error;
> -	struct overlay_registers __iomem *regs;
>  
>  	if (!overlay || !overlay->active)
>  		return NULL;
> @@ -1541,18 +1472,9 @@ intel_overlay_capture_error_state(struct drm_i915_private *dev_priv)
>  	error->isr = I915_READ(ISR);
>  	error->base = overlay->flip_addr;
>  
> -	regs = intel_overlay_map_regs_atomic(overlay);
> -	if (!regs)
> -		goto err;
> -
> -	memcpy_fromio(&error->regs, regs, sizeof(struct overlay_registers));
> -	intel_overlay_unmap_regs_atomic(overlay, regs);
> +	memcpy_fromio(&error->regs, overlay->regs, sizeof(error->regs));
>  
>  	return error;
> -
> -err:
> -	kfree(error);
> -	return NULL;
>  }
>  
>  void
> -- 
> 2.19.0.rc2

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