| Message ID | 20180521200113.GA20998@jordon-HP-15-Notebook-PC (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Souptick,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.17-rc6 next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Souptick-Joarder/gpu-drm-omapdrm-Adding-new-typedef-vm_fault_t/20180522-135920
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
drivers/gpu//drm/omapdrm/omap_gem.c: In function 'fault_2d':
drivers/gpu//drm/omapdrm/omap_gem.c:480:9: error: implicit declaration of function 'vmf_error'; did you mean '__pmd_error'? [-Werror=implicit-function-declaration]
ret = vmf_error(err);
^~~~~~~~~
__pmd_error
>> drivers/gpu//drm/omapdrm/omap_gem.c:503:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
return ret;
^~~
cc1: some warnings being treated as errors
vim +/ret +503 drivers/gpu//drm/omapdrm/omap_gem.c
398
399 /* Special handling for the case of faulting in 2d tiled buffers */
400 static vm_fault_t fault_2d(struct drm_gem_object *obj,
401 struct vm_area_struct *vma, struct vm_fault *vmf)
402 {
403 struct omap_gem_object *omap_obj = to_omap_bo(obj);
404 struct omap_drm_private *priv = obj->dev->dev_private;
405 struct omap_drm_usergart_entry *entry;
406 enum tiler_fmt fmt = gem2fmt(omap_obj->flags);
407 struct page *pages[64]; /* XXX is this too much to have on stack? */
408 unsigned long pfn;
409 pgoff_t pgoff, base_pgoff;
410 unsigned long vaddr;
411 int i, err, slots;
412 vm_fault_t ret;
413
414 /*
415 * Note the height of the slot is also equal to the number of pages
416 * that need to be mapped in to fill 4kb wide CPU page. If the slot
417 * height is 64, then 64 pages fill a 4kb wide by 64 row region.
418 */
419 const int n = priv->usergart[fmt].height;
420 const int n_shift = priv->usergart[fmt].height_shift;
421
422 /*
423 * If buffer width in bytes > PAGE_SIZE then the virtual stride is
424 * rounded up to next multiple of PAGE_SIZE.. this need to be taken
425 * into account in some of the math, so figure out virtual stride
426 * in pages
427 */
428 const int m = DIV_ROUND_UP(omap_obj->width << fmt, PAGE_SIZE);
429
430 /* We don't use vmf->pgoff since that has the fake offset: */
431 pgoff = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
432
433 /*
434 * Actual address we start mapping at is rounded down to previous slot
435 * boundary in the y direction:
436 */
437 base_pgoff = round_down(pgoff, m << n_shift);
438
439 /* figure out buffer width in slots */
440 slots = omap_obj->width >> priv->usergart[fmt].slot_shift;
441
442 vaddr = vmf->address - ((pgoff - base_pgoff) << PAGE_SHIFT);
443
444 entry = &priv->usergart[fmt].entry[priv->usergart[fmt].last];
445
446 /* evict previous buffer using this usergart entry, if any: */
447 if (entry->obj)
448 evict_entry(entry->obj, fmt, entry);
449
450 entry->obj = obj;
451 entry->obj_pgoff = base_pgoff;
452
453 /* now convert base_pgoff to phys offset from virt offset: */
454 base_pgoff = (base_pgoff >> n_shift) * slots;
455
456 /* for wider-than 4k.. figure out which part of the slot-row we want: */
457 if (m > 1) {
458 int off = pgoff % m;
459 entry->obj_pgoff += off;
460 base_pgoff /= m;
461 slots = min(slots - (off << n_shift), n);
462 base_pgoff += off << n_shift;
463 vaddr += off << PAGE_SHIFT;
464 }
465
466 /*
467 * Map in pages. Beyond the valid pixel part of the buffer, we set
468 * pages[i] to NULL to get a dummy page mapped in.. if someone
469 * reads/writes it they will get random/undefined content, but at
470 * least it won't be corrupting whatever other random page used to
471 * be mapped in, or other undefined behavior.
472 */
473 memcpy(pages, &omap_obj->pages[base_pgoff],
474 sizeof(struct page *) * slots);
475 memset(pages + slots, 0,
476 sizeof(struct page *) * (n - slots));
477
478 err = tiler_pin(entry->block, pages, ARRAY_SIZE(pages), 0, true);
479 if (err) {
> 480 ret = vmf_error(err);
481 dev_err(obj->dev->dev, "failed to pin: %d\n", err);
482 return ret;
483 }
484
485 pfn = entry->dma_addr >> PAGE_SHIFT;
486
487 VERB("Inserting %p pfn %lx, pa %lx", (void *)vmf->address,
488 pfn, pfn << PAGE_SHIFT);
489
490 for (i = n; i > 0; i--) {
491 ret = vmf_insert_mixed(vma,
492 vaddr, __pfn_to_pfn_t(pfn, PFN_DEV));
493 if (ret & VM_FAULT_ERROR)
494 break;
495 pfn += priv->usergart[fmt].stride_pfn;
496 vaddr += PAGE_SIZE * m;
497 }
498
499 /* simple round-robin: */
500 priv->usergart[fmt].last = (priv->usergart[fmt].last + 1)
501 % NUM_USERGART_ENTRIES;
502
> 503 return ret;
504 }
505
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
> drivers/gpu//drm/omapdrm/omap_gem.c: In function 'fault_2d': > drivers/gpu//drm/omapdrm/omap_gem.c:480:9: error: implicit declaration of function 'vmf_error'; did you mean '__pmd_error'? [-Werror=implicit-function-declaration] > ret = vmf_error(err); I think, drm-next and next-20180517 tree doesn't have vmf_error() included. As of now it's only available in 4.17-rc6. > ^~~~~~~~~ > __pmd_error >>> drivers/gpu//drm/omapdrm/omap_gem.c:503:9: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized] > return ret; > ^~~ > cc1: some warnings being treated as errors > I will fix this in v2.
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 0faf042..f9fe755 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -371,7 +371,7 @@ size_t omap_gem_mmap_size(struct drm_gem_object *obj) */ /* Normal handling for the case of faulting in non-tiled buffers */ -static int fault_1d(struct drm_gem_object *obj, +static vm_fault_t fault_1d(struct drm_gem_object *obj, struct vm_area_struct *vma, struct vm_fault *vmf) { struct omap_gem_object *omap_obj = to_omap_bo(obj); @@ -392,11 +392,12 @@ static int fault_1d(struct drm_gem_object *obj, VERB("Inserting %p pfn %lx, pa %lx", (void *)vmf->address, pfn, pfn << PAGE_SHIFT); - return vm_insert_mixed(vma, vmf->address, __pfn_to_pfn_t(pfn, PFN_DEV)); + return vmf_insert_mixed(vma, vmf->address, + __pfn_to_pfn_t(pfn, PFN_DEV)); } /* Special handling for the case of faulting in 2d tiled buffers */ -static int fault_2d(struct drm_gem_object *obj, +static vm_fault_t fault_2d(struct drm_gem_object *obj, struct vm_area_struct *vma, struct vm_fault *vmf) { struct omap_gem_object *omap_obj = to_omap_bo(obj); @@ -407,7 +408,8 @@ static int fault_2d(struct drm_gem_object *obj, unsigned long pfn; pgoff_t pgoff, base_pgoff; unsigned long vaddr; - int i, ret, slots; + int i, err, slots; + vm_fault_t ret; /* * Note the height of the slot is also equal to the number of pages @@ -473,9 +475,10 @@ static int fault_2d(struct drm_gem_object *obj, memset(pages + slots, 0, sizeof(struct page *) * (n - slots)); - ret = tiler_pin(entry->block, pages, ARRAY_SIZE(pages), 0, true); - if (ret) { - dev_err(obj->dev->dev, "failed to pin: %d\n", ret); + err = tiler_pin(entry->block, pages, ARRAY_SIZE(pages), 0, true); + if (err) { + ret = vmf_error(err); + dev_err(obj->dev->dev, "failed to pin: %d\n", err); return ret; } @@ -485,7 +488,10 @@ static int fault_2d(struct drm_gem_object *obj, pfn, pfn << PAGE_SHIFT); for (i = n; i > 0; i--) { - vm_insert_mixed(vma, vaddr, __pfn_to_pfn_t(pfn, PFN_DEV)); + ret = vmf_insert_mixed(vma, + vaddr, __pfn_to_pfn_t(pfn, PFN_DEV)); + if (ret & VM_FAULT_ERROR) + break; pfn += priv->usergart[fmt].stride_pfn; vaddr += PAGE_SIZE * m; } @@ -494,7 +500,7 @@ static int fault_2d(struct drm_gem_object *obj, priv->usergart[fmt].last = (priv->usergart[fmt].last + 1) % NUM_USERGART_ENTRIES; - return 0; + return ret; } /** @@ -509,14 +515,15 @@ static int fault_2d(struct drm_gem_object *obj, * vma->vm_private_data points to the GEM object that is backing this * mapping. */ -int omap_gem_fault(struct vm_fault *vmf) +vm_fault_t omap_gem_fault(struct vm_fault *vmf) { struct vm_area_struct *vma = vmf->vma; struct drm_gem_object *obj = vma->vm_private_data; struct omap_gem_object *omap_obj = to_omap_bo(obj); struct drm_device *dev = obj->dev; struct page **pages; - int ret; + int err; + vm_fault_t ret; /* Make sure we don't parallel update on a fault, nor move or remove * something from beneath our feet @@ -524,9 +531,11 @@ int omap_gem_fault(struct vm_fault *vmf) mutex_lock(&dev->struct_mutex); /* if a shmem backed object, make sure we have pages attached now */ - ret = get_pages(obj, &pages); - if (ret) + err = get_pages(obj, &pages); + if (err) { + ret = vmf_error(err); goto fail; + } /* where should we do corresponding put_pages().. we are mapping * the original page, rather than thru a GART, so we can't rely @@ -542,21 +551,7 @@ int omap_gem_fault(struct vm_fault *vmf) fail: mutex_unlock(&dev->struct_mutex); - switch (ret) { - case 0: - case -ERESTARTSYS: - case -EINTR: - case -EBUSY: - /* - * EBUSY is ok: this just means that another thread - * already did the job. - */ - return VM_FAULT_NOPAGE; - case -ENOMEM: - return VM_FAULT_OOM; - default: - return VM_FAULT_SIGBUS; - } + return ret; } /** We override mainly to fix up some of the vm mapping flags.. */ diff --git a/drivers/gpu/drm/omapdrm/omap_gem.h b/drivers/gpu/drm/omapdrm/omap_gem.h index a78bde0..c1c45fb 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.h +++ b/drivers/gpu/drm/omapdrm/omap_gem.h @@ -21,6 +21,7 @@ #define __OMAPDRM_GEM_H__ #include <linux/types.h> +#include <linux/mm_types.h> enum dma_data_direction; @@ -80,7 +81,7 @@ struct dma_buf *omap_gem_prime_export(struct drm_device *dev, struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev, struct dma_buf *buffer); -int omap_gem_fault(struct vm_fault *vmf); +vm_fault_t omap_gem_fault(struct vm_fault *vmf); int omap_gem_roll(struct drm_gem_object *obj, u32 roll); void omap_gem_cpu_sync_page(struct drm_gem_object *obj, int pgoff); void omap_gem_dma_sync_buffer(struct drm_gem_object *obj,
Use new return type vm_fault_t for fault handler. For now, this is just documenting that the function returns a VM_FAULT value rather than an errno. Once all instances are converted, vm_fault_t will become a distinct type. Ref-> commit 1c8f422059ae ("mm: change return type to vm_fault_t") Previously vm_insert_mixed() returns err which driver mapped into VM_FAULT_* type. Also return value of vm_insert_mixed() not handled correctly and 0 was returned inside fault_2d() as default. The new function vmf_insert_mixed() will replace this inefficiency by returning correct VM_FAULT_* type. vmf_error() is the newly introduce inline function in 4.17-rc6. Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com> --- drivers/gpu/drm/omapdrm/omap_gem.c | 51 +++++++++++++++++--------------------- drivers/gpu/drm/omapdrm/omap_gem.h | 3 ++- 2 files changed, 25 insertions(+), 29 deletions(-)