Re: [PATCH 4/9] drm: Begin an API for in-kernel clients

From: "Noralf Trønnes" <noralf@tronnes.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org,
	laurent.pinchart@ideasonboard.com,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 4/9] drm: Begin an API for in-kernel clients
Date: Mon, 28 May 2018 15:26:48 +0200	[thread overview]
Message-ID: <f6ae662f-a608-9f14-5273-973d6515fdb3@tronnes.org> (raw)
In-Reply-To: <20180524084213.GG3438@phenom.ffwll.local>

Den 24.05.2018 10.42, skrev Daniel Vetter:
> On Wed, May 23, 2018 at 04:34:06PM +0200, Noralf Trønnes wrote:
>> This the beginning of an API for in-kernel clients.
>> First out is a way to get a framebuffer backed by a dumb buffer.
>>
>> Only GEM drivers are supported.
>> The original idea of using an exported dma-buf was dropped because it
>> also creates an anonomous file descriptor which doesn't work when the
>> buffer is created from a kernel thread. The easy way out is to use
>> drm_driver.gem_prime_vmap to get the virtual address, which requires a
>> GEM object. This excludes the vmwgfx driver which is the only non-GEM
>> driver apart from the legacy ones. A solution for vmwgfx will have to be
>> worked out later if it wants to support the client API which it probably
>> will when we have a bootsplash client.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> A few small nits below, with those addressed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---

[...]

>> +/**
>> + * drm_client_new - Create a DRM client
>> + * @dev: DRM device
>> + *
>> + * Returns:
>> + * Pointer to a client or an error pointer on failure.
>> + */
>> +struct drm_client_dev *drm_client_new(struct drm_device *dev)
> Api nitpick:
>
> int drm_client_init(struct drm_device *dev,
> 		    struct drm_client_dev *client)
>
> and dropping the kzalloc from this structure here. This allows users of
> this to embed the client struct into their own thing, which means the
> ->private backpointer isn't necessary. Allowing embedding is the preferred
> interface in the kernel (since it's strictly more powerful, you can always
> just kzalloc + _init to get the _new behaviour).
>
>> +{
>> +	struct drm_client_dev *client;
>> +	int ret;
>> +
>> +	if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
>> +	    !dev->driver->dumb_create || !dev->driver->gem_prime_vmap)
>> +		return ERR_PTR(-ENOTSUPP);
>> +
>> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
>> +	if (!client)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	client->dev = dev;
>> +
>> +	ret = drm_client_alloc_file(client);
>> +	if (ret) {
>> +		kfree(client);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return client;
>> +}
>> +EXPORT_SYMBOL(drm_client_new);
>> +

[...]

>> +static struct drm_client_buffer *
>> +drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format)
>> +{
>> +	struct drm_mode_create_dumb dumb_args = { };
>> +	struct drm_device *dev = client->dev;
>> +	struct drm_client_buffer *buffer;
>> +	struct drm_gem_object *obj;
>> +	void *vaddr;
>> +	int ret;
>> +
>> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
>> +	if (!buffer)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	buffer->client = client;
>> +
>> +	dumb_args.width = width;
>> +	dumb_args.height = height;
>> +	dumb_args.bpp = drm_format_plane_cpp(format, 0) * 8;
>> +	ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
>> +	if (ret)
>> +		goto err_free;
>> +
>> +	buffer->handle = dumb_args.handle;
>> +	buffer->pitch = dumb_args.pitch;
>> +
>> +	obj = drm_gem_object_lookup(client->file, dumb_args.handle);
>> +	if (!obj)  {
>> +		ret = -ENOENT;
>> +		goto err_delete;
>> +	}
>> +
>> +	buffer->gem = obj;
>> +
> I'm paranoid, I think an
>
> 	if (WARN_ON(!gem_prime_vmap))
> 		return -EINVAL;
>
> would be cool here.

This is already checked in drm_client_init().

Noralf.

> Also perhaps the following comment:
>
> 	/*
> 	 * FIXME: The dependency on GEM here isn't required, we could
> 	 * convert the driver handle to a dma-buf instead and use the
> 	 * backend-agnostic dma-buf vmap support instead. This would
> 	 * require that the handle2fd prime ioctl is reworked to pull the
> 	 * fd_install step out of the driver backend hooks, to make that
> 	 * final step optional for internal users.
> 	 */
>
>
>> +	vaddr = dev->driver->gem_prime_vmap(obj);
>> +	if (!vaddr) {
>> +		ret = -ENOMEM;
>> +		goto err_delete;
>> +	}
>> +
>> +	buffer->vaddr = vaddr;
>> +
>> +	return buffer;
>> +
>> +err_delete:
>> +	drm_client_buffer_delete(buffer);
>> +err_free:
>> +	kfree(buffer);
>> +
>> +	return ERR_PTR(ret);
>> +}

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx