| Message ID | 1456780572-19196-4-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alex, [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on v4.5-rc6 next-20160229] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Dave-Gordon/Reorganise-calls-to-vmap-GEM-objects/20160301-052138 base: git://anongit.freedesktop.org/drm-intel for-linux-next reproduce: make htmldocs All warnings (new ones prefixed by >>): include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_select_subconnector_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_mode_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_left_margin_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_right_margin_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_top_margin_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_bottom_margin_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_brightness_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_contrast_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_flicker_reduction_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_overscan_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_saturation_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_hue_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'scaling_mode_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'aspect_ratio_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'dirty_info_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'suggested_x_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'suggested_y_property' include/drm/drm_crtc.h:2144: warning: No description found for parameter 'allow_fb_modifiers' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count' include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count' drivers/gpu/drm/drm_dp_mst_topology.c:2364: warning: No description found for parameter 'connector' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid' include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock' include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work' drivers/gpu/drm/drm_dp_mst_topology.c:2364: warning: No description found for parameter 'connector' drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags' include/drm/drmP.h:168: warning: No description found for parameter 'fmt' include/drm/drmP.h:184: warning: No description found for parameter 'fmt' include/drm/drmP.h:202: warning: No description found for parameter 'fmt' include/drm/drmP.h:247: warning: No description found for parameter 'dev' include/drm/drmP.h:247: warning: No description found for parameter 'data' include/drm/drmP.h:247: warning: No description found for parameter 'file_priv' include/drm/drmP.h:280: warning: No description found for parameter 'ioctl' include/drm/drmP.h:280: warning: No description found for parameter '_func' include/drm/drmP.h:280: warning: No description found for parameter '_flags' include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data ' include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver ' include/drm/drmP.h:672: warning: cannot understand function prototype: 'struct drm_info_list ' include/drm/drmP.h:682: warning: cannot understand function prototype: 'struct drm_info_node ' include/drm/drmP.h:692: warning: cannot understand function prototype: 'struct drm_minor ' include/drm/drmP.h:740: warning: cannot understand function prototype: 'struct drm_device ' drivers/gpu/drm/i915/intel_runtime_pm.c:2286: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/intel_runtime_pm.c:2286: warning: No description found for parameter 'resume' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1245: warning: No description found for parameter 'rps' drivers/gpu/drm/i915/i915_gem.c:1461: warning: No description found for parameter 'req' drivers/gpu/drm/i915/i915_gem.c:1496: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:1496: warning: No description found for parameter 'readonly' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1727: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1727: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1727: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:2015: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:2015: warning: No description found for parameter 'size' drivers/gpu/drm/i915/i915_gem.c:2015: warning: No description found for parameter 'tiling_mode' drivers/gpu/drm/i915/i915_gem.c:2015: warning: No description found for parameter 'fenced' drivers/gpu/drm/i915/i915_gem.c:2015: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment' >> drivers/gpu/drm/i915/i915_gem.c:2420: warning: No description found for parameter 'nbytes' >> drivers/gpu/drm/i915/i915_gem.c:2420: warning: Excess function parameter 'len' description in 'i915_gem_object_vmap_range' drivers/gpu/drm/i915/i915_gem.c:2972: warning: No description found for parameter 'ring' drivers/gpu/drm/i915/i915_gem.c:3103: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3153: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:3153: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:3153: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:3153: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl' drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'vm' drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'ggtt_view' drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'alignment' drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'flags' drivers/gpu/drm/i915/i915_gem.c:3780: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3780: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_gem.c:3855: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3855: warning: No description found for parameter 'cache_level' drivers/gpu/drm/i915/i915_gem.c:4129: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:4129: warning: No description found for parameter 'write' drivers/gpu/drm/i915/intel_lrc.c:936: warning: No description found for parameter 'params' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dev' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'file' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ring' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: No description found for parameter 'params' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dev' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'file' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ring' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission' Warning: didn't use docs for i915_hotplug_interrupt_update Warning: didn't use docs for ilk_update_display_irq Warning: didn't use docs for ilk_update_gt_irq Warning: didn't use docs for snb_update_pm_irq Warning: didn't use docs for bdw_update_port_irq Warning: didn't use docs for bdw_update_pipe_irq Warning: didn't use docs for ibx_display_interrupt_update Warning: didn't use docs for i915_enable_asle_pipestat Warning: didn't use docs for ivybridge_parity_work Warning: didn't use docs for i915_reset_and_wakeup Warning: didn't use docs for i915_handle_error Warning: didn't use docs for intel_irq_install Warning: didn't use docs for intel_irq_uninstall vim +/nbytes +2420 drivers/gpu/drm/i915/i915_gem.c 2404 * i915_gem_object_vmap_range - map some or all of a GEM object into kernel space 2405 * @obj: the GEM object to be mapped 2406 * @start: offset in bytes of the start of the range to be mapped 2407 * @len: length in bytes of the range to be mapped 2408 * 2409 * Map a given range of a GEM object into kernel virtual space. The range will 2410 * be extended at both ends, if necessary, to span a whole number of pages. The 2411 * caller must make sure the associated pages are gathered and pinned before 2412 * calling this function, and is responsible for unmapping the returned address 2413 * when it is no longer required. 2414 * 2415 * Returns the address at which the object has been mapped, or NULL on failure. 2416 */ 2417 void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj, 2418 unsigned long start, 2419 unsigned long nbytes) > 2420 { 2421 struct scatterlist *sg = obj->pages->sgl; 2422 struct sg_page_iter sg_iter; 2423 struct page **pages; 2424 unsigned long first, npages, i; 2425 int nents; 2426 void *addr; 2427 2428 /* Check requested range against underlying sg list */ --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 29/02/16 21:16, Dave Gordon wrote: > From: Alex Dai <yu.dai@intel.com> > > There are several places inside driver where a GEM object is mapped > to kernel virtual space. The mapping may be done either for the whole > object or only a subset of it. > > This patch introduces a function i915_gem_object_vmap_range() to > implement the common functionality. The code itself is extracted and > adapted from that in vmap_batch(), but also replaces vmap_obj() and the > open-coded version in i915_gem_dmabuf_vmap(). > > v2: use obj->pages->nents for iteration within i915_gem_object_vmap; > break when it finishes all desired pages. The caller must pass the > actual page count required. [Tvrtko Ursulin] > > v4: renamed to i915_gem_object_vmap_range() to make its function > clearer. [Dave Gordon] > > v5: use Chris Wilson's new drm_malloc_gfp() rather than kmalloc() or > drm_malloc_ab(). [Dave Gordon] > > v6: changed range checking to not use pages->nents. [Tvrtko Ursulin] > Use sg_nents_for_len() for range check instead. [Dave Gordon] > Pass range parameters in bytes rather than pages (both callers > were converting from bytes to pages anyway, so this reduces the > number of places where the conversion is done). > > With this change, we have only one vmap() in the whole driver :) > > Signed-off-by: Alex Dai <yu.dai@intel.com> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 32 +----------------- > drivers/gpu/drm/i915/i915_drv.h | 4 +++ > drivers/gpu/drm/i915/i915_gem.c | 58 +++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 16 ++------- > drivers/gpu/drm/i915/intel_ringbuffer.c | 24 ++------------ > 5 files changed, 68 insertions(+), 66 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 814d894..1b2515d 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -863,37 +863,7 @@ void i915_cmd_parser_fini_ring(struct intel_engine_cs *ring) > static u32 *vmap_batch(struct drm_i915_gem_object *obj, > unsigned start, unsigned len) > { > - int i; > - void *addr = NULL; > - struct sg_page_iter sg_iter; > - int first_page = start >> PAGE_SHIFT; > - int last_page = (len + start + 4095) >> PAGE_SHIFT; > - int npages = last_page - first_page; > - struct page **pages; > - > - pages = drm_malloc_ab(npages, sizeof(*pages)); > - if (pages == NULL) { > - DRM_DEBUG_DRIVER("Failed to get space for pages\n"); > - goto finish; > - } > - > - i = 0; > - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, first_page) { > - pages[i++] = sg_page_iter_page(&sg_iter); > - if (i == npages) > - break; > - } > - > - addr = vmap(pages, i, 0, PAGE_KERNEL); > - if (addr == NULL) { > - DRM_DEBUG_DRIVER("Failed to vmap pages\n"); > - goto finish; > - } > - > -finish: > - if (pages) > - drm_free_large(pages); > - return (u32*)addr; > + return i915_gem_object_vmap_range(obj, start, len); > } > > /* Returns a vmap'd pointer to dest_obj, which the caller must unmap */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a4dcb74..12b0717 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2983,6 +2983,10 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) > obj->pages_pin_count--; > } > > +void *__must_check i915_gem_object_vmap_range(struct drm_i915_gem_object *obj, > + unsigned long start, > + unsigned long nbytes); > + > int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); > int i915_gem_object_sync(struct drm_i915_gem_object *obj, > struct intel_engine_cs *to, > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 3d31d3a..8f12e73 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2400,6 +2400,64 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj) > return 0; > } > > +/** > + * i915_gem_object_vmap_range - map some or all of a GEM object into kernel space > + * @obj: the GEM object to be mapped > + * @start: offset in bytes of the start of the range to be mapped > + * @len: length in bytes of the range to be mapped kbuild spotted this kerneldoc issue. > + * > + * Map a given range of a GEM object into kernel virtual space. The range will > + * be extended at both ends, if necessary, to span a whole number of pages. The > + * caller must make sure the associated pages are gathered and pinned before > + * calling this function, and is responsible for unmapping the returned address > + * when it is no longer required. > + * > + * Returns the address at which the object has been mapped, or NULL on failure. > + */ > +void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj, > + unsigned long start, > + unsigned long nbytes) > +{ > + struct scatterlist *sg = obj->pages->sgl; > + struct sg_page_iter sg_iter; > + struct page **pages; > + unsigned long first, npages, i; > + int nents; > + void *addr; > + > + /* Check requested range against underlying sg list */ > + nents = sg_nents_for_len(sg, start + nbytes); > + if (nents < 0) { > + DRM_DEBUG_DRIVER("Invalid page count\n"); > + return NULL; > + } I think this is needless overhead. The helper will iterate the whole sg chain while we know the size in obj->base.size and finding out the real nents is of little (no) use to the code below. > + > + /* Work in pages from now on */ > + first = start >> PAGE_SHIFT; > + npages = DIV_ROUND_UP(start + nbytes, PAGE_SIZE) - first; And this looks like weak API if the caller can pass non page aligned start and size and the function will silently vmap something else. It should assert and fail on both I think, or it may have been simpler to keep it working in page units. > + > + pages = drm_malloc_gfp(npages, sizeof(*pages), GFP_TEMPORARY); > + if (pages == NULL) { > + DRM_DEBUG_DRIVER("Failed to get space for pages\n"); > + return NULL; > + } > + > + i = 0; > + for_each_sg_page(sg, &sg_iter, nents, first) { > + pages[i] = sg_page_iter_page(&sg_iter); > + if (++i >= npages) > + break; > + } > + WARN_ON(i != npages); > + > + addr = vmap(pages, npages, 0, PAGE_KERNEL); > + if (addr == NULL) > + DRM_DEBUG_DRIVER("Failed to vmap pages\n"); > + drm_free_large(pages); > + > + return addr; > +} > + > void i915_vma_move_to_active(struct i915_vma *vma, > struct drm_i915_gem_request *req) > { > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index 1f3eef6..aee4149 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -110,9 +110,7 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) > { > struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); > struct drm_device *dev = obj->base.dev; > - struct sg_page_iter sg_iter; > - struct page **pages; > - int ret, i; > + int ret; > > ret = i915_mutex_lock_interruptible(dev); > if (ret) > @@ -131,16 +129,8 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) > > ret = -ENOMEM; > > - pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages)); > - if (pages == NULL) > - goto err_unpin; > - > - i = 0; > - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) > - pages[i++] = sg_page_iter_page(&sg_iter); > - > - obj->dma_buf_vmapping = vmap(pages, i, 0, PAGE_KERNEL); > - drm_free_large(pages); > + obj->dma_buf_vmapping = i915_gem_object_vmap_range(obj, 0, > + dma_buf->size >> PAGE_SHIFT); This is still in pages. (Although as said below I think it should remain and API be reverted back.) > > if (!obj->dma_buf_vmapping) > goto err_unpin; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 45ce45a..434a452 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2064,27 +2064,6 @@ void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > i915_gem_object_ggtt_unpin(ringbuf->obj); > } > > -static u32 *vmap_obj(struct drm_i915_gem_object *obj) > -{ > - struct sg_page_iter sg_iter; > - struct page **pages; > - void *addr; > - int i; > - > - pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages)); > - if (pages == NULL) > - return NULL; > - > - i = 0; > - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) > - pages[i++] = sg_page_iter_page(&sg_iter); > - > - addr = vmap(pages, i, 0, PAGE_KERNEL); > - drm_free_large(pages); > - > - return addr; > -} > - > int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, > struct intel_ringbuffer *ringbuf) > { > @@ -2103,7 +2082,8 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, > return ret; > } > > - ringbuf->virtual_start = vmap_obj(obj); > + ringbuf->virtual_start = i915_gem_object_vmap_range(obj, 0, > + ringbuf->size >> PAGE_SHIFT); Here also. > if (ringbuf->virtual_start == NULL) { > i915_gem_object_ggtt_unpin(obj); > return -ENOMEM; > Regards, Tvrtko
On 01/03/16 10:12, Tvrtko Ursulin wrote: > > On 29/02/16 21:16, Dave Gordon wrote: >> From: Alex Dai <yu.dai@intel.com> >> >> There are several places inside driver where a GEM object is mapped >> to kernel virtual space. The mapping may be done either for the whole >> object or only a subset of it. >> >> This patch introduces a function i915_gem_object_vmap_range() to >> implement the common functionality. The code itself is extracted and >> adapted from that in vmap_batch(), but also replaces vmap_obj() and the >> open-coded version in i915_gem_dmabuf_vmap(). >> >> v2: use obj->pages->nents for iteration within i915_gem_object_vmap; >> break when it finishes all desired pages. The caller must pass the >> actual page count required. [Tvrtko Ursulin] >> >> v4: renamed to i915_gem_object_vmap_range() to make its function >> clearer. [Dave Gordon] >> >> v5: use Chris Wilson's new drm_malloc_gfp() rather than kmalloc() or >> drm_malloc_ab(). [Dave Gordon] >> >> v6: changed range checking to not use pages->nents. [Tvrtko Ursulin] >> Use sg_nents_for_len() for range check instead. [Dave Gordon] >> Pass range parameters in bytes rather than pages (both callers >> were converting from bytes to pages anyway, so this reduces the >> number of places where the conversion is done). >> >> With this change, we have only one vmap() in the whole driver :) >> >> Signed-off-by: Alex Dai <yu.dai@intel.com> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- [snip] >> +/** >> + * i915_gem_object_vmap_range - map some or all of a GEM object into >> kernel space >> + * @obj: the GEM object to be mapped >> + * @start: offset in bytes of the start of the range to be mapped >> + * @len: length in bytes of the range to be mapped > > kbuild spotted this kerneldoc issue. Ah yes, that parameter got renamed >> + * >> + * Map a given range of a GEM object into kernel virtual space. The >> range will >> + * be extended at both ends, if necessary, to span a whole number of >> pages. The >> + * caller must make sure the associated pages are gathered and pinned >> before >> + * calling this function, and is responsible for unmapping the >> returned address >> + * when it is no longer required. >> + * >> + * Returns the address at which the object has been mapped, or NULL >> on failure. >> + */ >> +void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj, >> + unsigned long start, >> + unsigned long nbytes) >> +{ >> + struct scatterlist *sg = obj->pages->sgl; >> + struct sg_page_iter sg_iter; >> + struct page **pages; >> + unsigned long first, npages, i; >> + int nents; >> + void *addr; >> + >> + /* Check requested range against underlying sg list */ >> + nents = sg_nents_for_len(sg, start + nbytes); >> + if (nents < 0) { >> + DRM_DEBUG_DRIVER("Invalid page count\n"); >> + return NULL; >> + } > > I think this is needless overhead. The helper will iterate the whole sg > chain while we know the size in obj->base.size and finding out the real > nents is of little (no) use to the code below. It was more of a consistency check between the GEM object on the one hand, and the SGlist implementation on the other; since none of the other SG interfaces actually work in any useful quantities (e.g. pages or bytes; 'nents' is particularly useless, as it is affected by the details of the way the underlying pages have been mapped, in particular how many separate chunks they're in). >> + >> + /* Work in pages from now on */ >> + first = start >> PAGE_SHIFT; >> + npages = DIV_ROUND_UP(start + nbytes, PAGE_SIZE) - first; > > And this looks like weak API if the caller can pass non page aligned > start and size and the function will silently vmap something else. > > It should assert and fail on both I think, or it may have been simpler > to keep it working in page units. It is exactly the API required by copy_batch() (which is where this code came from), and it's a perfectly well-defined API: the start offset is rounded down to the start of the containing page, the end address (start + len) is rounded up to the next page boundary, and that defines what gets mapped i.e. the smallest page-aligned region containing the range specified. But I could let (all) the callers do that conversion instead ... >> + pages = drm_malloc_gfp(npages, sizeof(*pages), GFP_TEMPORARY); >> + if (pages == NULL) { >> + DRM_DEBUG_DRIVER("Failed to get space for pages\n"); >> + return NULL; >> + } >> + >> + i = 0; >> + for_each_sg_page(sg, &sg_iter, nents, first) { >> + pages[i] = sg_page_iter_page(&sg_iter); >> + if (++i >= npages) >> + break; >> + } >> + WARN_ON(i != npages); >> + >> + addr = vmap(pages, npages, 0, PAGE_KERNEL); >> + if (addr == NULL) >> + DRM_DEBUG_DRIVER("Failed to vmap pages\n"); >> + drm_free_large(pages); >> + >> + return addr; >> +} >> + >> void i915_vma_move_to_active(struct i915_vma *vma, >> struct drm_i915_gem_request *req) >> { >> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> b/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> index 1f3eef6..aee4149 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> @@ -110,9 +110,7 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf >> *dma_buf) >> { >> struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); >> struct drm_device *dev = obj->base.dev; >> - struct sg_page_iter sg_iter; >> - struct page **pages; >> - int ret, i; >> + int ret; >> >> ret = i915_mutex_lock_interruptible(dev); >> if (ret) >> @@ -131,16 +129,8 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf >> *dma_buf) >> >> ret = -ENOMEM; >> >> - pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages)); >> - if (pages == NULL) >> - goto err_unpin; >> - >> - i = 0; >> - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) >> - pages[i++] = sg_page_iter_page(&sg_iter); >> - >> - obj->dma_buf_vmapping = vmap(pages, i, 0, PAGE_KERNEL); >> - drm_free_large(pages); >> + obj->dma_buf_vmapping = i915_gem_object_vmap_range(obj, 0, >> + dma_buf->size >> PAGE_SHIFT); > > This is still in pages. (Although as said below I think it should remain > and API be reverted back.) Ah yes, I missed this call and the one below, 'cos they get fixed up by the last patch of the series (which *does* adopt the new interface). >> if (!obj->dma_buf_vmapping) >> goto err_unpin; >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c >> b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index 45ce45a..434a452 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -2064,27 +2064,6 @@ void intel_unpin_ringbuffer_obj(struct >> intel_ringbuffer *ringbuf) >> i915_gem_object_ggtt_unpin(ringbuf->obj); >> } >> >> -static u32 *vmap_obj(struct drm_i915_gem_object *obj) >> -{ >> - struct sg_page_iter sg_iter; >> - struct page **pages; >> - void *addr; >> - int i; >> - >> - pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages)); >> - if (pages == NULL) >> - return NULL; >> - >> - i = 0; >> - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) >> - pages[i++] = sg_page_iter_page(&sg_iter); >> - >> - addr = vmap(pages, i, 0, PAGE_KERNEL); >> - drm_free_large(pages); >> - >> - return addr; >> -} >> - >> int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, >> struct intel_ringbuffer *ringbuf) >> { >> @@ -2103,7 +2082,8 @@ int intel_pin_and_map_ringbuffer_obj(struct >> drm_device *dev, >> return ret; >> } >> >> - ringbuf->virtual_start = vmap_obj(obj); >> + ringbuf->virtual_start = i915_gem_object_vmap_range(obj, 0, >> + ringbuf->size >> PAGE_SHIFT); > > Here also. Actually ... if we work in pages but adopt a convention that length==0 means "the whole object" then only copy_batch() would have to do any page-aligning calculations; all other callers always want the whole object so it's nice to make the interface convenient for them :) I'll see how that works out ... .Dave. >> if (ringbuf->virtual_start == NULL) { >> i915_gem_object_ggtt_unpin(obj); >> return -ENOMEM; >> > > Regards, > Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 814d894..1b2515d 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -863,37 +863,7 @@ void i915_cmd_parser_fini_ring(struct intel_engine_cs *ring) static u32 *vmap_batch(struct drm_i915_gem_object *obj, unsigned start, unsigned len) { - int i; - void *addr = NULL; - struct sg_page_iter sg_iter; - int first_page = start >> PAGE_SHIFT; - int last_page = (len + start + 4095) >> PAGE_SHIFT; - int npages = last_page - first_page; - struct page **pages; - - pages = drm_malloc_ab(npages, sizeof(*pages)); - if (pages == NULL) { - DRM_DEBUG_DRIVER("Failed to get space for pages\n"); - goto finish; - } - - i = 0; - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, first_page) { - pages[i++] = sg_page_iter_page(&sg_iter); - if (i == npages) - break; - } - - addr = vmap(pages, i, 0, PAGE_KERNEL); - if (addr == NULL) { - DRM_DEBUG_DRIVER("Failed to vmap pages\n"); - goto finish; - } - -finish: - if (pages) - drm_free_large(pages); - return (u32*)addr; + return i915_gem_object_vmap_range(obj, start, len); } /* Returns a vmap'd pointer to dest_obj, which the caller must unmap */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a4dcb74..12b0717 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2983,6 +2983,10 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) obj->pages_pin_count--; } +void *__must_check i915_gem_object_vmap_range(struct drm_i915_gem_object *obj, + unsigned long start, + unsigned long nbytes); + int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); int i915_gem_object_sync(struct drm_i915_gem_object *obj, struct intel_engine_cs *to, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3d31d3a..8f12e73 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2400,6 +2400,64 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj) return 0; } +/** + * i915_gem_object_vmap_range - map some or all of a GEM object into kernel space + * @obj: the GEM object to be mapped + * @start: offset in bytes of the start of the range to be mapped + * @len: length in bytes of the range to be mapped + * + * Map a given range of a GEM object into kernel virtual space. The range will + * be extended at both ends, if necessary, to span a whole number of pages. The + * caller must make sure the associated pages are gathered and pinned before + * calling this function, and is responsible for unmapping the returned address + * when it is no longer required. + * + * Returns the address at which the object has been mapped, or NULL on failure. + */ +void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj, + unsigned long start, + unsigned long nbytes) +{ + struct scatterlist *sg = obj->pages->sgl; + struct sg_page_iter sg_iter; + struct page **pages; + unsigned long first, npages, i; + int nents; + void *addr; + + /* Check requested range against underlying sg list */ + nents = sg_nents_for_len(sg, start + nbytes); + if (nents < 0) { + DRM_DEBUG_DRIVER("Invalid page count\n"); + return NULL; + } + + /* Work in pages from now on */ + first = start >> PAGE_SHIFT; + npages = DIV_ROUND_UP(start + nbytes, PAGE_SIZE) - first; + + pages = drm_malloc_gfp(npages, sizeof(*pages), GFP_TEMPORARY); + if (pages == NULL) { + DRM_DEBUG_DRIVER("Failed to get space for pages\n"); + return NULL; + } + + i = 0; + for_each_sg_page(sg, &sg_iter, nents, first) { + pages[i] = sg_page_iter_page(&sg_iter); + if (++i >= npages) + break; + } + WARN_ON(i != npages); + + addr = vmap(pages, npages, 0, PAGE_KERNEL); + if (addr == NULL) + DRM_DEBUG_DRIVER("Failed to vmap pages\n"); + drm_free_large(pages); + + return addr; +} + void i915_vma_move_to_active(struct i915_vma *vma, struct drm_i915_gem_request *req) { diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 1f3eef6..aee4149 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -110,9 +110,7 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) { struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); struct drm_device *dev = obj->base.dev; - struct sg_page_iter sg_iter; - struct page **pages; - int ret, i; + int ret; ret = i915_mutex_lock_interruptible(dev); if (ret) @@ -131,16 +129,8 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) ret = -ENOMEM; - pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages)); - if (pages == NULL) - goto err_unpin; - - i = 0; - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) - pages[i++] = sg_page_iter_page(&sg_iter); - - obj->dma_buf_vmapping = vmap(pages, i, 0, PAGE_KERNEL); - drm_free_large(pages); + obj->dma_buf_vmapping = i915_gem_object_vmap_range(obj, 0, + dma_buf->size >> PAGE_SHIFT); if (!obj->dma_buf_vmapping) goto err_unpin; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 45ce45a..434a452 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2064,27 +2064,6 @@ void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf) i915_gem_object_ggtt_unpin(ringbuf->obj); } -static u32 *vmap_obj(struct drm_i915_gem_object *obj) -{ - struct sg_page_iter sg_iter; - struct page **pages; - void *addr; - int i; - - pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages)); - if (pages == NULL) - return NULL; - - i = 0; - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) - pages[i++] = sg_page_iter_page(&sg_iter); - - addr = vmap(pages, i, 0, PAGE_KERNEL); - drm_free_large(pages); - - return addr; -} - int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, struct intel_ringbuffer *ringbuf) { @@ -2103,7 +2082,8 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, return ret; } - ringbuf->virtual_start = vmap_obj(obj); + ringbuf->virtual_start = i915_gem_object_vmap_range(obj, 0, + ringbuf->size >> PAGE_SHIFT); if (ringbuf->virtual_start == NULL) { i915_gem_object_ggtt_unpin(obj); return -ENOMEM;