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@xxxxxxxxxxx> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > +#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@xxxxxxxxxx>"); > +MODULE_LICENSE("GPL"); > -- > 2.21.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
