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

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

Den 14.09.2018 12.10, skrev Eric Engestrom:
> 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.

Yeah, the helper was called drm_shem_bo_helper in an earlier version,
forgot to remove the BO from the enum.

Noralf.

> > > +	}
> > > +
> > > +	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