Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Daniel Vetter]

On Fri, May 06, 2016 at 04:34:08PM +0200, Noralf Trønnes wrote:
> 
> Den 06.05.2016 16:15, skrev Thierry Reding:
> >On Fri, May 06, 2016 at 04:08:16PM +0200, Daniel Vetter wrote:
> >>On Fri, May 06, 2016 at 04:03:47PM +0200, Thierry Reding wrote:
> >>>On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> >>>>Add function to create a simple connector for a panel.
> >>>I'm not sure I see the usefulness of this. Typically you'd attach a
> >>>panel to an encoder/connector, in which case you already have the
> >>>connector.
> >>>
> >>>Perhaps it would become more obvious why we need this if you posted
> >>>patches that show where this is used?
> >>The other helpers give you a simple drm pipeline with plane, crtc &
> >>encoder all baked into on drm_simple_pipeline structure. The only thing
> >>variable you have to hook up to that is the drm_connector. And I think for
> >>dead-simple panels avoiding the basic boilerplate in that does indeed make
> >>some sense.
> >Avoiding boilerplate is good, but I have a difficult time envisioning
> >how you might want to use this. At the same time I'm asking myself how
> >we know that this helper is any good if we haven't seen it used anywhere
> >and actually see the boilerplate go away.
> 
> I pulled out the patches from the tinydrm patchset that would go
> into drm core and helpers. I'm not very good at juggling many patches
> around in various version and getting it right.
> I'm doing development in the downstream Raspberry Pi repo
> on 4.5 to get all the pi drivers, and then apply it on linux-next...
> 
> This is the tinydrm patch that will use it:
> https://lists.freedesktop.org/archives/dri-devel/2016-April/104502.html
> 
> Extract:
> +int tinydrm_display_pipe_init(struct tinydrm_device *tdev,
> +                  const uint32_t *formats, unsigned int format_count)
> +{
> +    struct drm_device *dev = tdev->base;
> +    struct drm_connector *connector;
> +    int ret;
> +
> +    connector = drm_simple_kms_panel_connector_create(dev, &tdev->panel,
> +                        DRM_MODE_CONNECTOR_VIRTUAL);
> +    if (IS_ERR(connector))
> +        return PTR_ERR(connector);
> +
> +    ret = drm_simple_display_pipe_init(dev, &tdev->pipe,
> +                &tinydrm_display_pipe_funcs,
> +                formats, format_count,
> +                connector);
> +
> +    return ret;
> +}
> +EXPORT_SYMBOL(tinydrm_display_pipe_init);

It's imo even more impressive when you show the entire driver, including
all the hooks. Because there's just 4 of them iirc, all optional.

And besides the tinydrm panel drivers in it's various editions we could
also use this for simpledrm (the boot-up firmware kms driver from David
Herrmann) and probably a few other super-simple display drivers.
Essentially with this I think drm is now better suited for dumb&simple
hardware than fbdev.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch