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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Apr 16, 2018 at 05:58:23PM +0200, Noralf Trønnes wrote:
> 
> 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).

Ah yes, I missed that.

Wrt mmap, not sure we should use the dma-buf mmap or the dumb mmap. I
guess in the end it wont matter much really.

> 
> 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);

Nah, if we need the dma-buf anyway, I'd try to go directly from the handle
to the dma-buf. So roughly:

	ret = drm_mode_create_dumb(dev, &dumb_args, file);
	
	dma_buf = drm_gem_prime_handle_to_dmabuf(file, dumb_args.handle);

See my reply to the ioctl wrapper patch for details.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx