On Fri, Apr 18, 2014 at 08:27:03AM +0100, Chris Wilson wrote: > A single object may be referenced by multiple registers fundamentally > breaking the static allotment of ids in the current design. When the > object is used the second time, the physical address of the first > assignment is relinquished and a second one granted. However, the > hardware is still reading (and possibly writing) to the old physical > address now returned to the system. Eventually hilarity will ensue, but > in the short term, it just means that cursors are broken when using more > than one pipe. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77351 > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > --- > drivers/gpu/drm/i915/i915_dma.c | 1 - > drivers/gpu/drm/i915/i915_drv.h | 24 +-- > drivers/gpu/drm/i915/i915_gem.c | 315 ++++++++++++++--------------------- > drivers/gpu/drm/i915/intel_display.c | 11 +- > drivers/gpu/drm/i915/intel_overlay.c | 12 +- > 5 files changed, 131 insertions(+), 232 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index d67ca8051e07..12571aac7516 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1854,7 +1854,6 @@ int i915_driver_unload(struct drm_device *dev) > flush_workqueue(dev_priv->wq); > > mutex_lock(&dev->struct_mutex); > - i915_gem_free_all_phys_object(dev); > i915_gem_cleanup_ringbuffer(dev); > i915_gem_context_fini(dev); > WARN_ON(dev_priv->mm.aliasing_ppgtt); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 0e85bf2c3326..a2375a0ef27c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -246,18 +246,6 @@ struct intel_ddi_plls { > #define WATCH_LISTS 0 > #define WATCH_GTT 0 > > -#define I915_GEM_PHYS_CURSOR_0 1 > -#define I915_GEM_PHYS_CURSOR_1 2 > -#define I915_GEM_PHYS_OVERLAY_REGS 3 > -#define I915_MAX_PHYS_OBJECT (I915_GEM_PHYS_OVERLAY_REGS) > - > -struct drm_i915_gem_phys_object { > - int id; > - struct page **page_list; > - drm_dma_handle_t *handle; > - struct drm_i915_gem_object *cur_obj; > -}; > - > struct opregion_header; > struct opregion_acpi; > struct opregion_swsci; > @@ -1036,9 +1024,6 @@ struct i915_gem_mm { > /** Bit 6 swizzling required for Y tiling */ > uint32_t bit_6_swizzle_y; > > - /* storage for physical objects */ > - struct drm_i915_gem_phys_object *phys_objs[I915_MAX_PHYS_OBJECT]; > - > /* accounting, useful for userland debugging */ > spinlock_t object_stat_lock; > size_t object_memory; > @@ -1647,7 +1632,7 @@ struct drm_i915_gem_object { > struct drm_file *pin_filp; > > /** for phy allocated objects */ > - struct drm_i915_gem_phys_object *phys_obj; > + drm_dma_handle_t *phys_handle; > > union { > struct i915_gem_userptr { > @@ -2250,13 +2235,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, > u32 alignment, > struct intel_ring_buffer *pipelined); > void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj); > -int i915_gem_attach_phys_object(struct drm_device *dev, > - struct drm_i915_gem_object *obj, > - int id, > +int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, > int align); > -void i915_gem_detach_phys_object(struct drm_device *dev, > - struct drm_i915_gem_object *obj); > -void i915_gem_free_all_phys_object(struct drm_device *dev); > int i915_gem_open(struct drm_device *dev, struct drm_file *file); > void i915_gem_release(struct drm_device *dev, struct drm_file *file); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 764467ebade5..e302a23031fe 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -46,11 +46,6 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj, > static void > i915_gem_object_retire(struct drm_i915_gem_object *obj); > > -static int i915_gem_phys_pwrite(struct drm_device *dev, > - struct drm_i915_gem_object *obj, > - struct drm_i915_gem_pwrite *args, > - struct drm_file *file); > - > static void i915_gem_write_fence(struct drm_device *dev, int reg, > struct drm_i915_gem_object *obj); > static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj, > @@ -212,6 +207,124 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, > return 0; > } > > +static void i915_gem_object_detach_phys(struct drm_i915_gem_object *obj) > +{ > + drm_dma_handle_t *phys = obj->phys_handle; > + > + if (!phys) > + return; > + > + if (obj->madv == I915_MADV_WILLNEED) { > + struct address_space *mapping = file_inode(obj->base.filp)->i_mapping; > + char *vaddr = phys->vaddr; > + int i; > + > + for (i = 0; i < obj->base.size / PAGE_SIZE; i++) { > + struct page *page = shmem_read_mapping_page(mapping, i); > + if (!IS_ERR(page)) { > + char *dst = kmap_atomic(page); > + memcpy(dst, vaddr, PAGE_SIZE); > + kunmap_atomic(dst); > + > + drm_clflush_pages(&page, 1); Could clflush while already have it kmapped to avoid double kmap. > + > + set_page_dirty(page); > + mark_page_accessed(page); > + page_cache_release(page); > + } > + vaddr += PAGE_SIZE; > + } > + i915_gem_chipset_flush(obj->base.dev); > + } > + > +#ifdef CONFIG_X86 > + set_memory_wb((unsigned long)phys->vaddr, phys->size / PAGE_SIZE); > +#endif > + drm_pci_free(obj->base.dev, phys); > + obj->phys_handle = NULL; > +} > + > +int > +i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, > + int align) > +{ > + drm_dma_handle_t *phys; > + struct address_space *mapping; > + char *vaddr; > + int i; > + > + if (obj->phys_handle) { > + if ((unsigned long)obj->phys_handle->vaddr & (align -1)) > + return -EBUSY; > + > + return 0; > + } > + > + if (obj->madv != I915_MADV_WILLNEED) > + return -EFAULT; > + > + if (obj->base.filp == NULL) > + return -EINVAL; > + > + /* create a new object */ > + phys = drm_pci_alloc(obj->base.dev, obj->base.size, align); > + if (!phys) > + return -ENOMEM; > + > + vaddr = phys->vaddr; > +#ifdef CONFIG_X86 > + set_memory_wc((unsigned long)vaddr, phys->size / PAGE_SIZE); > +#endif > + mapping = file_inode(obj->base.filp)->i_mapping; > + for (i = 0; i < obj->base.size / PAGE_SIZE; i++) { > + struct page *page; > + char *src; > + > + page = shmem_read_mapping_page(mapping, i); > + if (IS_ERR(page)) > + return PTR_ERR(page); This is going to leak. > + > + src = kmap_atomic(page); > + memcpy(vaddr, src, PAGE_SIZE); > + kunmap_atomic(src); > + > + mark_page_accessed(page); > + page_cache_release(page); > + > + vaddr += PAGE_SIZE; > + } > + > + obj->phys_handle = phys; > + return 0; > +} > + > +static int > +i915_gem_phys_pwrite(struct drm_i915_gem_object *obj, > + struct drm_i915_gem_pwrite *args, > + struct drm_file *file_priv) > +{ > + struct drm_device *dev = obj->base.dev; > + void *vaddr = obj->phys_handle->vaddr + args->offset; > + char __user *user_data = to_user_ptr(args->data_ptr); > + > + if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) { > + unsigned long unwritten; > + > + /* The physical object once assigned is fixed for the lifetime > + * of the obj, so we can safely drop the lock and continue > + * to access vaddr. > + */ > + mutex_unlock(&dev->struct_mutex); > + unwritten = copy_from_user(vaddr, user_data, args->size); > + mutex_lock(&dev->struct_mutex); > + if (unwritten) > + return -EFAULT; I was wondering why we don't clflush here but then I realized the destination is either wc or uc. Apart from the potential leak the rest of patch looks OK to me. > + } > + > + i915_gem_chipset_flush(dev); > + return 0; > +} > + > void *i915_gem_object_alloc(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -1071,8 +1184,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, > * pread/pwrite currently are reading and writing from the CPU > * perspective, requiring manual detiling by the client. > */ > - if (obj->phys_obj) { > - ret = i915_gem_phys_pwrite(dev, obj, args, file); > + if (obj->phys_handle) { > + ret = i915_gem_phys_pwrite(obj, args, file); > goto out; > } > > @@ -4376,9 +4489,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > > trace_i915_gem_object_destroy(obj); > > - if (obj->phys_obj) > - i915_gem_detach_phys_object(dev, obj); > - > list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) { > int ret; > > @@ -4408,6 +4518,7 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > i915_gem_object_put_pages(obj); > i915_gem_object_free_mmap_offset(obj); > i915_gem_object_release_stolen(obj); > + i915_gem_object_detach_phys(obj); > > BUG_ON(obj->pages); > > @@ -4875,190 +4986,6 @@ i915_gem_load(struct drm_device *dev) > register_shrinker(&dev_priv->mm.shrinker); > } > > -/* > - * Create a physically contiguous memory object for this object > - * e.g. for cursor + overlay regs > - */ > -static int i915_gem_init_phys_object(struct drm_device *dev, > - int id, int size, int align) > -{ > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct drm_i915_gem_phys_object *phys_obj; > - int ret; > - > - if (dev_priv->mm.phys_objs[id - 1] || !size) > - return 0; > - > - phys_obj = kzalloc(sizeof(*phys_obj), GFP_KERNEL); > - if (!phys_obj) > - return -ENOMEM; > - > - phys_obj->id = id; > - > - phys_obj->handle = drm_pci_alloc(dev, size, align); > - if (!phys_obj->handle) { > - ret = -ENOMEM; > - goto kfree_obj; > - } > -#ifdef CONFIG_X86 > - set_memory_wc((unsigned long)phys_obj->handle->vaddr, phys_obj->handle->size / PAGE_SIZE); > -#endif > - > - dev_priv->mm.phys_objs[id - 1] = phys_obj; > - > - return 0; > -kfree_obj: > - kfree(phys_obj); > - return ret; > -} > - > -static void i915_gem_free_phys_object(struct drm_device *dev, int id) > -{ > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct drm_i915_gem_phys_object *phys_obj; > - > - if (!dev_priv->mm.phys_objs[id - 1]) > - return; > - > - phys_obj = dev_priv->mm.phys_objs[id - 1]; > - if (phys_obj->cur_obj) { > - i915_gem_detach_phys_object(dev, phys_obj->cur_obj); > - } > - > -#ifdef CONFIG_X86 > - set_memory_wb((unsigned long)phys_obj->handle->vaddr, phys_obj->handle->size / PAGE_SIZE); > -#endif > - drm_pci_free(dev, phys_obj->handle); > - kfree(phys_obj); > - dev_priv->mm.phys_objs[id - 1] = NULL; > -} > - > -void i915_gem_free_all_phys_object(struct drm_device *dev) > -{ > - int i; > - > - for (i = I915_GEM_PHYS_CURSOR_0; i <= I915_MAX_PHYS_OBJECT; i++) > - i915_gem_free_phys_object(dev, i); > -} > - > -void i915_gem_detach_phys_object(struct drm_device *dev, > - struct drm_i915_gem_object *obj) > -{ > - struct address_space *mapping = file_inode(obj->base.filp)->i_mapping; > - char *vaddr; > - int i; > - int page_count; > - > - if (!obj->phys_obj) > - return; > - vaddr = obj->phys_obj->handle->vaddr; > - > - page_count = obj->base.size / PAGE_SIZE; > - for (i = 0; i < page_count; i++) { > - struct page *page = shmem_read_mapping_page(mapping, i); > - if (!IS_ERR(page)) { > - char *dst = kmap_atomic(page); > - memcpy(dst, vaddr + i*PAGE_SIZE, PAGE_SIZE); > - kunmap_atomic(dst); > - > - drm_clflush_pages(&page, 1); > - > - set_page_dirty(page); > - mark_page_accessed(page); > - page_cache_release(page); > - } > - } > - i915_gem_chipset_flush(dev); > - > - obj->phys_obj->cur_obj = NULL; > - obj->phys_obj = NULL; > -} > - > -int > -i915_gem_attach_phys_object(struct drm_device *dev, > - struct drm_i915_gem_object *obj, > - int id, > - int align) > -{ > - struct address_space *mapping = file_inode(obj->base.filp)->i_mapping; > - struct drm_i915_private *dev_priv = dev->dev_private; > - int ret = 0; > - int page_count; > - int i; > - > - if (id > I915_MAX_PHYS_OBJECT) > - return -EINVAL; > - > - if (obj->phys_obj) { > - if (obj->phys_obj->id == id) > - return 0; > - i915_gem_detach_phys_object(dev, obj); > - } > - > - /* create a new object */ > - if (!dev_priv->mm.phys_objs[id - 1]) { > - ret = i915_gem_init_phys_object(dev, id, > - obj->base.size, align); > - if (ret) { > - DRM_ERROR("failed to init phys object %d size: %zu\n", > - id, obj->base.size); > - return ret; > - } > - } > - > - /* bind to the object */ > - obj->phys_obj = dev_priv->mm.phys_objs[id - 1]; > - obj->phys_obj->cur_obj = obj; > - > - page_count = obj->base.size / PAGE_SIZE; > - > - for (i = 0; i < page_count; i++) { > - struct page *page; > - char *dst, *src; > - > - page = shmem_read_mapping_page(mapping, i); > - if (IS_ERR(page)) > - return PTR_ERR(page); > - > - src = kmap_atomic(page); > - dst = obj->phys_obj->handle->vaddr + (i * PAGE_SIZE); > - memcpy(dst, src, PAGE_SIZE); > - kunmap_atomic(src); > - > - mark_page_accessed(page); > - page_cache_release(page); > - } > - > - return 0; > -} > - > -static int > -i915_gem_phys_pwrite(struct drm_device *dev, > - struct drm_i915_gem_object *obj, > - struct drm_i915_gem_pwrite *args, > - struct drm_file *file_priv) > -{ > - void *vaddr = obj->phys_obj->handle->vaddr + args->offset; > - char __user *user_data = to_user_ptr(args->data_ptr); > - > - if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) { > - unsigned long unwritten; > - > - /* The physical object once assigned is fixed for the lifetime > - * of the obj, so we can safely drop the lock and continue > - * to access vaddr. > - */ > - mutex_unlock(&dev->struct_mutex); > - unwritten = copy_from_user(vaddr, user_data, args->size); > - mutex_lock(&dev->struct_mutex); > - if (unwritten) > - return -EFAULT; > - } > - > - i915_gem_chipset_flush(dev); > - return 0; > -} > - > void i915_gem_release(struct drm_device *dev, struct drm_file *file) > { > struct drm_i915_file_private *file_priv = file->driver_priv; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3d017f5b656a..61735fde3f12 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7777,14 +7777,12 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > addr = i915_gem_obj_ggtt_offset(obj); > } else { > int align = IS_I830(dev) ? 16 * 1024 : 256; > - ret = i915_gem_attach_phys_object(dev, obj, > - (intel_crtc->pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1, > - align); > + 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_obj->handle->busaddr; > + addr = obj->phys_handle->busaddr; > } > > if (IS_GEN2(dev)) > @@ -7792,10 +7790,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > > finish: > if (intel_crtc->cursor_bo) { > - if (INTEL_INFO(dev)->cursor_needs_physical) { > - if (intel_crtc->cursor_bo != obj) > - i915_gem_detach_phys_object(dev, intel_crtc->cursor_bo); > - } else > + if (!INTEL_INFO(dev)->cursor_needs_physical) > i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo); > drm_gem_object_unreference(&intel_crtc->cursor_bo->base); > } > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c > index 4232230a78bd..5423eb0c1335 100644 > --- a/drivers/gpu/drm/i915/intel_overlay.c > +++ b/drivers/gpu/drm/i915/intel_overlay.c > @@ -194,7 +194,7 @@ intel_overlay_map_regs(struct intel_overlay *overlay) > struct overlay_registers __iomem *regs; > > if (OVERLAY_NEEDS_PHYSICAL(overlay->dev)) > - regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_obj->handle->vaddr; > + regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr; > else > regs = io_mapping_map_wc(dev_priv->gtt.mappable, > i915_gem_obj_ggtt_offset(overlay->reg_bo)); > @@ -1347,14 +1347,12 @@ void intel_setup_overlay(struct drm_device *dev) > overlay->reg_bo = reg_bo; > > if (OVERLAY_NEEDS_PHYSICAL(dev)) { > - ret = i915_gem_attach_phys_object(dev, reg_bo, > - I915_GEM_PHYS_OVERLAY_REGS, > - PAGE_SIZE); > + 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_obj->handle->busaddr; > + overlay->flip_addr = reg_bo->phys_handle->busaddr; > } else { > ret = i915_gem_obj_ggtt_pin(reg_bo, PAGE_SIZE, PIN_MAPPABLE); > if (ret) { > @@ -1436,7 +1434,7 @@ intel_overlay_map_regs_atomic(struct intel_overlay *overlay) > /* 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_obj->handle->vaddr; > + overlay->reg_bo->phys_handle->vaddr; > else > regs = io_mapping_map_atomic_wc(dev_priv->gtt.mappable, > i915_gem_obj_ggtt_offset(overlay->reg_bo)); > @@ -1470,7 +1468,7 @@ intel_overlay_capture_error_state(struct drm_device *dev) > error->dovsta = I915_READ(DOVSTA); > error->isr = I915_READ(ISR); > if (OVERLAY_NEEDS_PHYSICAL(overlay->dev)) > - error->base = (__force long)overlay->reg_bo->phys_obj->handle->vaddr; > + error->base = (__force long)overlay->reg_bo->phys_handle->vaddr; > else > error->base = i915_gem_obj_ggtt_offset(overlay->reg_bo); > > -- > 1.9.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
