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

[Noralf Trønnes]

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 at 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.

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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel