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 <nor...@tronnes.org>
> Signed-off-by: Hans de Goede <hdego...@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 <hdego...@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
