On Wednesday, 2018-09-12 20:33:32 -0500, David Lechner wrote:
> On 09/11/2018 07:43 AM, Noralf Trønnes wrote:
> > This adds a library for shmem backed GEM objects with the necessary
> > drm_driver callbacks.
> >
> > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > ---
> >
>
> ...
>
> > +static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem)
> > +{
> > + struct drm_gem_object *obj = &shmem->base;
> > + int ret;
> > +
> > + if (shmem->vmap_use_count++ > 0)
> > + return 0;
> > +
> > + ret = drm_gem_shmem_get_pages(shmem);
> > + if (ret)
> > + goto err_zero_use;
> > +
> > + if (obj->import_attach) {
> > + shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf);
> > + } else {
> > + pgprot_t prot;
> > +
> > + switch (shmem->cache_mode) {
> > + case DRM_GEM_SHMEM_BO_UNKNOWN:
> > + DRM_DEBUG_KMS("Can't vmap cache mode is unknown\n");
> > + ret = -EINVAL;
> > + goto err_put_pages;
> > +
> > + case DRM_GEM_SHMEM_BO_WRITECOMBINED:
> > + prot = pgprot_writecombine(PAGE_KERNEL);
> > + break;
> > +
> > + case DRM_GEM_SHMEM_BO_UNCACHED:
> > + prot = pgprot_noncached(PAGE_KERNEL);
> > + break;
> > +
> > + case DRM_GEM_SHMEM_BO_CACHED:
> > + prot = PAGE_KERNEL;
> > + break;
> > + }
> > +
> > + shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, VM_MAP, prot);
>
> I get a gcc warning here:
>
> /home/david/work/ev3dev2/ev3dev-kernel/drivers/gpu/drm/drm_gem_shmem_helper.c:220:18: warning: ‘prot’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, VM_MAP, prot);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/david/work/ev3dev2/ev3dev-kernel/drivers/gpu/drm/drm_gem_shmem_helper.c:199:12: note: ‘prot’ was declared here
> pgprot_t prot;
> ^~~~
This warning is saying that the vmap could be reached without hitting
any cases in the switch, which is technically true if one sets the
cache_mode to some garbage, but not if only existing enums from `enum
drm_gem_shmem_cache_mode` are used.
I think we should just add a `default:` next to the
DRM_GEM_SHMEM_BO_UNKNOWN case.
>
> ---
>
> And since I am making a comment anyway, it is not clear to me
> what BO means in the enum names. I didn't see any hints in the
> doc comments either.
Buffer Object, but yeah I guess it's not necessary in the enum names.
>
>
> > + }
> > +
> > + if (!shmem->vaddr) {
> > + DRM_DEBUG_KMS("Failed to vmap pages\n");
> > + ret = -ENOMEM;
> > + goto err_put_pages;
> > + }
> > +
> > + return 0;
> > +
> > +err_put_pages:
> > + drm_gem_shmem_put_pages(shmem);
> > +err_zero_use:
> > + shmem->vmap_use_count = 0;
> > +
> > + return ret;
> > +}
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
