Re: [PATCH 4/9] drm: Begin an API for in-kernel clients

[Daniel Vetter <daniel@ffwll.ch>]

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@tronnes.org>
> > A few small nits below, with those addressed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> 
> [...]
> 
> > > +/**
> > > + * 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx