[Intel-gfx] [RFC v4 19/25] drm/client: Finish the in-kernel client API

[Noralf Trønnes]

Den 16.04.2018 10.27, skrev Daniel Vetter:
> On Sat, Apr 14, 2018 at 01:53:12PM +0200, Noralf Trønnes wrote:
>> The modesetting code is already present, this adds the rest of the API.
> Mentioning the TODO in the commit message would be good. Helps readers
> like me who have an attention span measured in seconds :-)
>
> Just commenting on the create_buffer leak here
>
>> +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_prime_handle prime_args = { };
>> +	struct drm_client_buffer *buffer;
>> +	struct dma_buf *dma_buf;
>> +	void *vaddr;
>> +	int ret;
>> +
>> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
>> +	if (!buffer)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	buffer->client = client;
>> +	buffer->width = width;
>> +	buffer->height = height;
>> +	buffer->format = format;
>> +
>> +	dumb_args.width = buffer->width;
>> +	dumb_args.height = buffer->height;
>> +	dumb_args.bpp = drm_format_plane_cpp(format, 0) * 8;
>> +	ret = drm_mode_create_dumb(client->dev, &dumb_args, client->file);
>> +	if (ret)
>> +		goto err_free;
>> +
>> +	buffer->handle = dumb_args.handle;
>> +	buffer->pitch = dumb_args.pitch;
>> +	buffer->size = dumb_args.size;
>> +
>> +	prime_args.handle = dumb_args.handle;
>> +	ret = drm_prime_handle_to_fd(client->dev, &prime_args, client->file);
>> +	if (ret)
>> +		goto err_delete;
>> +
>> +	dma_buf = dma_buf_get(prime_args.fd);
>> +	if (IS_ERR(dma_buf)) {
>> +		ret = PTR_ERR(dma_buf);
>> +		goto err_delete;
>> +	}
>> +
>> +	/*
>> +	 * If called from a worker the dmabuf fd isn't closed and the ref
>> +	 * doesn't drop to zero on free.
>> +	 * If I use __close_fd() it's all fine, but that function is not exported.
>> +	 *
>> +	 * How do I get rid of this fd when in a worker/kernel thread?
>> +	 * The fd isn't used beyond this function.
>> +	 */
>> +//	WARN_ON(__close_fd(current->files, prime_args.fd));
> Hm, this isn't 100% what I had in mind as the sequence for generic buffer
> creation. Pseudo-code:
>
> 	ret = drm_mode_create_dumb(client->dev, &dumb_args, client->file);
> 	if (ret)
> 		goto err_free;
> 	
> 	gem_bo = drm_gem_object_lookup(client->file, dumb_args.handle);
>
> gives you _really_ directly the underlying gem_bo. Of course this doesn't
> work for non-gem based driver, but reality is that (almost) all of them
> are. And we will not accept any new drivers which aren't gem based. So
> ignoring vmwgfx for this drm_client work is imo perfectly fine. We should
> ofc keep the option in the fb helpers to use non-gem buffers (so that
> vmwgfx could switch over from their own in-driver fbdev helpers). All we
> need for that is to keep the fb_probe callback.
>
> Was there any other reason than vmwgfx for using prime buffers instead of
> just directly using gem?

The reason for using a prime buffer is that it gives me easy access to a
dma_buf which I use to get the virtual address (dma_buf_vmap) and for
mmap (dma_buf_mmap).

Would this stripped down version of drm_gem_prime_handle_to_fd() work?

struct dma_buf *drm_gem_to_dmabuf(struct drm_gem_object *obj)
{
     struct dma_buf *dmabuf;

     mutex_lock(&obj->dev->object_name_lock);
     /* re-export the original imported object */
     if (obj->import_attach) {
         dmabuf = obj->import_attach->dmabuf;
         get_dma_buf(dmabuf);
         goto out;
     }

     if (obj->dma_buf) {
         dmabuf = obj->dma_buf;
         get_dma_buf(dmabuf);
         goto out;
     }

     dmabuf = export_and_register_object(obj->dev, obj, 0);
out:
     mutex_unlock(&obj->dev->object_name_lock);

     return dmabuf;
}

Now I could do this:

     ret = drm_mode_create_dumb(dev, &dumb_args, file);

     obj = drm_gem_object_lookup(file, dumb_args.handle);

     dmabuf = drm_gem_to_dmabuf(obj);

     vaddr = dma_buf_vmap(dmabuf);


Noralf.