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?
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
