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.