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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
