On Mon, May 28, 2018 at 03:26:48PM +0200, Noralf Trønnes wrote:
>
> Den 24.05.2018 10.42, skrev Daniel Vetter:
> > On Wed, May 23, 2018 at 04:34:06PM +0200, Noralf Trønnes wrote:
> > > This the beginning of an API for in-kernel clients.
> > > First out is a way to get a framebuffer backed by a dumb buffer.
> > >
> > > Only GEM drivers are supported.
> > > The original idea of using an exported dma-buf was dropped because it
> > > also creates an anonomous file descriptor which doesn't work when the
> > > buffer is created from a kernel thread. The easy way out is to use
> > > drm_driver.gem_prime_vmap to get the virtual address, which requires a
> > > GEM object. This excludes the vmwgfx driver which is the only non-GEM
> > > driver apart from the legacy ones. A solution for vmwgfx will have to be
> > > worked out later if it wants to support the client API which it probably
> > > will when we have a bootsplash client.
> > >
> > > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > A few small nits below, with those addressed:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > ---
>
> [...]
>
> > > +/**
> > > + * drm_client_new - Create a DRM client
> > > + * @dev: DRM device
> > > + *
> > > + * Returns:
> > > + * Pointer to a client or an error pointer on failure.
> > > + */
> > > +struct drm_client_dev *drm_client_new(struct drm_device *dev)
> > Api nitpick:
> >
> > int drm_client_init(struct drm_device *dev,
> > struct drm_client_dev *client)
> >
> > and dropping the kzalloc from this structure here. This allows users of
> > this to embed the client struct into their own thing, which means the
> > ->private backpointer isn't necessary. Allowing embedding is the preferred
> > interface in the kernel (since it's strictly more powerful, you can always
> > just kzalloc + _init to get the _new behaviour).
> >
> > > +{
> > > + struct drm_client_dev *client;
> > > + int ret;
> > > +
> > > + if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
> > > + !dev->driver->dumb_create || !dev->driver->gem_prime_vmap)
> > > + return ERR_PTR(-ENOTSUPP);
> > > +
> > > + client = kzalloc(sizeof(*client), GFP_KERNEL);
> > > + if (!client)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + client->dev = dev;
> > > +
> > > + ret = drm_client_alloc_file(client);
> > > + if (ret) {
> > > + kfree(client);
> > > + return ERR_PTR(ret);
> > > + }
> > > +
> > > + return client;
> > > +}
> > > +EXPORT_SYMBOL(drm_client_new);
> > > +
>
> [...]
>
> > > +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_device *dev = client->dev;
> > > + struct drm_client_buffer *buffer;
> > > + struct drm_gem_object *obj;
> > > + void *vaddr;
> > > + int ret;
> > > +
> > > + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> > > + if (!buffer)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + buffer->client = client;
> > > +
> > > + dumb_args.width = width;
> > > + dumb_args.height = height;
> > > + dumb_args.bpp = drm_format_plane_cpp(format, 0) * 8;
> > > + ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
> > > + if (ret)
> > > + goto err_free;
> > > +
> > > + buffer->handle = dumb_args.handle;
> > > + buffer->pitch = dumb_args.pitch;
> > > +
> > > + obj = drm_gem_object_lookup(client->file, dumb_args.handle);
> > > + if (!obj) {
> > > + ret = -ENOENT;
> > > + goto err_delete;
> > > + }
> > > +
> > > + buffer->gem = obj;
> > > +
> > I'm paranoid, I think an
> >
> > if (WARN_ON(!gem_prime_vmap))
> > return -EINVAL;
> >
> > would be cool here.
>
> This is already checked in drm_client_init().
Yeah I noticed later on. I think rechecking for safety here can't hurt,
but up to you.
-Daniel
>
> Noralf.
>
> > Also perhaps the following comment:
> >
> > /*
> > * FIXME: The dependency on GEM here isn't required, we could
> > * convert the driver handle to a dma-buf instead and use the
> > * backend-agnostic dma-buf vmap support instead. This would
> > * require that the handle2fd prime ioctl is reworked to pull the
> > * fd_install step out of the driver backend hooks, to make that
> > * final step optional for internal users.
> > */
> >
> >
> > > + vaddr = dev->driver->gem_prime_vmap(obj);
> > > + if (!vaddr) {
> > > + ret = -ENOMEM;
> > > + goto err_delete;
> > > + }
> > > +
> > > + buffer->vaddr = vaddr;
> > > +
> > > + return buffer;
> > > +
> > > +err_delete:
> > > + drm_client_buffer_delete(buffer);
> > > +err_free:
> > > + kfree(buffer);
> > > +
> > > + return ERR_PTR(ret);
> > > +}
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
