Re: [PATCH] drm: Add Grain Media GM12U320 driver v2

[Sam Ravnborg <sam@ravnborg.org>]

Hi Hans.

Driver looks good. Nce to see so much functionality in a driver less
than 1000 loc.

Following some bike-shedding only - that should have been sent for v1
already. But I thought better late then never.

	Sam

On Sun, Jul 21, 2019 at 03:25:25PM +0200, Hans de Goede wrote:
> Add a modesetting driver for Grain Media GM12U320 based devices
> (primarily Acer C120 projector, but there may be compatible devices).
> 
> This is based on the fb driver from Viacheslav Nurmekhamitov:
> https://github.com/slavrn/gm12u320
> 
> This driver uses drm_simple_display_pipe to deal with all the atomic
> stuff, gem_shmem_helper functions for buffer management and
> drm_fbdev_generic_setup for fbdev emulation, so that leaves the driver
> itself with only the actual code for talking to the gm13u320 chip,
> leading to a nice simple and clean driver.
> 
> Changes in v2:
> -Add drm-misc tree to MAINTAINERS
> -Drop mode_config.preferred_depth = 24 / fix fbdev support
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

> +#include <drm/drm_ioctl.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_vblank.h>
> +
> +static bool eco_mode;
> +module_param(eco_mode, bool, 0644);
> +MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less bright, more silent)");
> +
> +#define DRIVER_NAME		"gm12u320"
> +#define DRIVER_DESC		"Grain Media GM12U320 USB projector display"
> +#define DRIVER_DATE		"2019"
> +#define DRIVER_MAJOR		1
> +#define DRIVER_MINOR		0
> +#define DRIVER_PATCHLEVEL	1
DRIVER_PATCHLEVEL is not used

> +struct gm12u320_device {
> +	struct drm_device	         dev;
bike-shedding follows.
The name "drm" feels more natural for the drm device.
dev can then represent a struct device.

> +	struct drm_simple_display_pipe   pipe;
> +	struct drm_connector	         conn;
conn => connector

> +	struct usb_device               *udev;
udev => usb_dev

> +	unsigned char                   *cmd_buf;
> +	unsigned char                   *data_buf[GM12U320_BLOCK_COUNT];
> +	bool                             pipe_enabled;
> +	struct {
> +		bool                     run;
> +		struct workqueue_struct *workq;
> +		struct work_struct       work;
> +		wait_queue_head_t        waitq;
> +		struct mutex             lock;
> +		struct drm_framebuffer  *fb;
> +		struct drm_rect          rect;
> +	} fb_update;
> +};
> +

> +
> +static void gm12u320_32bpp_to_24bpp_packed(u8 *dst, u8 *src, int len)
> +{
> +	while (len--) {
> +		*dst++ = *src++;
> +		*dst++ = *src++;
> +		*dst++ = *src++;
> +		src++;
> +	}
> +}
Candidate for a generic function?

> +static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
> +{
> +	int block, dst_offset, len, remain, ret, x1, x2, y1, y2;
> +	struct drm_framebuffer *fb;
> +	void *vaddr;
> +	u8 *src;
> +
> +	mutex_lock(&gm12u320->fb_update.lock);
> +
> +	if (!gm12u320->fb_update.fb)
> +		goto unlock;
> +
> +	fb = gm12u320->fb_update.fb;
> +	x1 = gm12u320->fb_update.rect.x1;
> +	x2 = gm12u320->fb_update.rect.x2;
> +	y1 = gm12u320->fb_update.rect.y1;
> +	y2 = gm12u320->fb_update.rect.y2;
> +
> +	vaddr = drm_gem_shmem_vmap(fb->obj[0]);
> +	if (IS_ERR(vaddr)) {
> +		DRM_ERROR("failed to vmap fb: %ld\n", PTR_ERR(vaddr));
> +		goto put_fb;
> +	}
> +
> +	if (fb->obj[0]->import_attach) {
> +		ret = dma_buf_begin_cpu_access(
> +			fb->obj[0]->import_attach->dmabuf, DMA_FROM_DEVICE);
> +		if (ret) {
> +			DRM_ERROR("dma_buf_begin_cpu_access err: %d\n", ret);
> +			goto vunmap;
> +		}
> +	}
Here the code uses DRM_ERROR("...");


> +	/* Do not log errors caused by module unload or device unplug */
> +	if (ret != -ECONNRESET && ret != -ESHUTDOWN)
> +		dev_err(&gm12u320->udev->dev, "Frame update error: %d\n", ret);
> +}
And here dev_err(dev, "...");
It had been more consistent to use DRM_DEV_ERROR(dev, "..."); here.

> +/* ------------------------------------------------------------------ */
> +/* gm12u320 connector						      */
> +

> +
> +#ifdef CONFIG_PM
> +static int gm12u320_suspend(struct usb_interface *interface,
> +			    pm_message_t message)
> +{
You can use __maybe_unused to get rid of the #ifdef,
or you can ignore the slight codesize increase to get bettet build
coverage.

> +	struct drm_device *dev = usb_get_intfdata(interface);
> +	struct gm12u320_device *gm12u320 = dev->dev_private;
> +
> +	if (gm12u320->pipe_enabled)
> +		gm12u320_stop_fb_update(gm12u320);
> +
> +	return 0;
> +}
> +
> +static int gm12u320_resume(struct usb_interface *interface)
> +{
> +	struct drm_device *dev = usb_get_intfdata(interface);
> +	struct gm12u320_device *gm12u320 = dev->dev_private;
> +
> +	gm12u320_set_ecomode(gm12u320);
> +	if (gm12u320->pipe_enabled)
> +		gm12u320_start_fb_update(gm12u320);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct usb_device_id id_table[] = {
> +	{ USB_DEVICE(0x1de1, 0xc102) },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +static struct usb_driver gm12u320_usb_driver = {
> +	.name = "gm12u320",
> +	.probe = gm12u320_usb_probe,
> +	.disconnect = gm12u320_usb_disconnect,
> +	.id_table = id_table,
> +#ifdef CONFIG_PM
> +	.suspend = gm12u320_suspend,
> +	.resume = gm12u320_resume,
> +	.reset_resume = gm12u320_resume,
> +#endif
Unless you care about code-size, you can drop the ifdef section.

> +};
> +
> +module_usb_driver(gm12u320_usb_driver);
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.21.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel