Re: [PATCH v3 1/2] drm: Add library for shmem backed GEM objects

[Eric Engestrom <eric.engestrom@intel.com>]

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@tronnes.org>
> > ---
> > 
> 
> ...
> 
> > +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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel