Re: [RFC 1/5] drm: Add DRM support for tiny LCD displays

["Noralf Trønnes" <noralf@tronnes.org>]

Den 23.03.2016 18:37, skrev David Herrmann:
> Hey
>
> On Wed, Mar 16, 2016 at 2:34 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>> tinydrm provides a very simplified view of DRM for displays that has
>> onboard video memory and is connected through a slow bus like SPI/I2C.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---

[...]

>> +static struct drm_driver tinydrm_driver = {
>> +       .driver_features        = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME
>> +                               | DRIVER_ATOMIC,
>> +       .load                   = tinydrm_load,
>> +       .lastclose              = tinydrm_lastclose,
>> +//     .unload                 = tinydrm_unload,
>> +       .get_vblank_counter     = drm_vblank_count,
>> +//     .enable_vblank          = tinydrm_enable_vblank,
>> +//     .disable_vblank         = tinydrm_disable_vblank,
>> +       .gem_free_object        = drm_gem_cma_free_object,
>> +       .gem_vm_ops             = &drm_gem_cma_vm_ops,
>> +       .prime_handle_to_fd     = drm_gem_prime_handle_to_fd,
>> +       .prime_fd_to_handle     = drm_gem_prime_fd_to_handle,
>> +       .gem_prime_import       = drm_gem_prime_import,
>> +       .gem_prime_export       = drm_gem_prime_export,
>> +       .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
>> +       .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
>> +       .gem_prime_vmap         = drm_gem_cma_prime_vmap,
>> +       .gem_prime_vunmap       = drm_gem_cma_prime_vunmap,
>> +       .gem_prime_mmap         = drm_gem_cma_prime_mmap,
>> +       .dumb_create            = drm_gem_cma_dumb_create,
>> +       .dumb_map_offset        = drm_gem_cma_dumb_map_offset,
>> +       .dumb_destroy           = drm_gem_dumb_destroy,
>> +       .fops                   = &tinydrm_fops,
>> +       .name                   = "tinydrm",
>> +       .desc                   = "tinydrm",
>> +       .date                   = "20150916",
> Can we just drop "date" and "desc" from new drivers? It doesn't add any value.
>
>> +       .major                  = 1,
>> +       .minor                  = 0,
>> +};
>> +
>> +void tinydrm_release(struct tinydrm_device *tdev)
> We usually prefer "unregister()" to stay consistent with "register()".

Sure.

>> +{
>> +       DRM_DEBUG_KMS("\n");
>> +
>> +       if (tdev->deferred)
>> +               cancel_delayed_work_sync(&tdev->deferred->dwork);
>> +
>> +       tinydrm_fbdev_fini(tdev);
>> +
>> +       drm_panel_detach(&tdev->panel);
>> +       drm_panel_remove(&tdev->panel);
>> +
>> +       drm_mode_config_cleanup(tdev->base);
>> +       drm_dev_unregister(tdev->base);
>> +       drm_dev_unref(tdev->base);
>> +}
>> +EXPORT_SYMBOL(tinydrm_release);
>> +
>> +int tinydrm_register(struct device *dev, struct tinydrm_device *tdev)
>> +{
>> +       struct drm_driver *driver = &tinydrm_driver;
>> +       struct drm_device *ddev;
>> +       int ret;
>> +
>> +       dev_info(dev, "%s\n", __func__);
>> +
>> +dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +
>> +       if (WARN_ON(!tdev->dirtyfb))
>> +               return -EINVAL;
>> +
>> +       ddev = drm_dev_alloc(driver, dev);
>> +       if (!ddev)
>> +               return -ENOMEM;
>> +
>> +       tdev->base = ddev;
>> +       ddev->dev_private = tdev;
>> +
>> +       ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
>> +       if (ret)
>> +               goto err_free;
>> +
>> +       ret = drm_dev_register(ddev, 0);
>> +       if (ret)
>> +               goto err_free;
> Whatever your .load() callback does, do that here and drop it. It is
> really not needed. Optionally do it before calling drm_dev_register(),
> depending on which semantics you want.

Ok.

> In general, this looks very similar to what I did with SimpleDRM.

SimpleDRM was the first drm code I studied and tinydrm started with chunks
of code from it :-)

> However, I wonder whether we can make it more modular. Right now it
> always adds code for fbdev, CMA, backlight, etc., but as Daniel
> mentioned those better live in DRM-core helpers.

Yes I will make the next version more modular.
Instead of trying to guess which parts would fit as core helpers, I just
put everything into one module and posted an RFC hoping to get some 
feedback.
I will try an implement the suggestions Daniel has made.

> I'll try forward porting the SimpleDRM drivers to it, and let you know
> whether it works out.
>
> Thanks
> David


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel