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

Ville Syrjälä
2018-09-11T13:07:21Z

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