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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKMK7uGUW7Uzx5dYG9JxTTqQ4eg3SHirJKq0TLOcG3nnqbY9Bw@mail.gmail.com>
Date:	Sat, 7 May 2016 11:59:13 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Noralf Trønnes <noralf@...nnes.org>
Cc:	Thierry Reding <thierry.reding@...il.com>,
	Thierry Reding <treding@...dia.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

On Fri, May 6, 2016 at 9:45 PM, Noralf Trønnes <noralf@...nnes.org> wrote:
> In the discussion following the "no more fbdev drivers" call, someone
> pointed me to drm_panel. It had function hooks that I needed, so I built
> tinydrm around it. If this is misusing drm_panel, it shouldn't be too
> difficult to drop it.
>
> Actually I could just do this:
>
> struct tinydrm_funcs {
>         int (*prepare)(struct tinydrm_device *tdev);
>         int (*unprepare)(struct tinydrm_device *tdev);
>         int (*enable)(struct tinydrm_device *tdev);
>         int (*disable)(struct tinydrm_device *tdev);
>         int (*dirty)(struct drm_framebuffer *fb, void *vmem, unsigned flags,
>                      unsigned color, struct drm_clip_rect *clips,
>                      unsigned num_clips);
> };
>
>
> When it comes to the simple connector, one solution could be to extend
> drm_simple_display_pipe to embed a connector with (optional) modes:
>
>  struct drm_simple_display_pipe {
> -        struct drm_connector *connector;
> +        struct drm_connector connector;
> +        const struct drm_display_mode *modes;
> +        unsigned int num_modes;
>  };
>
> And maybe optional detect and get_modes hooks:
>
>  struct drm_simple_display_pipe_funcs {
> +        enum drm_connector_status detect(struct drm_connector *connector,
> +                                         bool force);
> +        int (*get_modes)(struct drm_connector *connector);
>  };
>
>  int drm_simple_display_pipe_init(struct drm_device *dev,
>                                   struct drm_simple_display_pipe *pipe,
>                                   struct drm_simple_display_pipe_funcs
> *funcs,
>                                   const uint32_t *formats,
>                                   unsigned int format_count,
> -                                 struct drm_connector *connector);
> +                                 int connector_type);

Tbh I really like that simple_display_pipe is split after the encoder.
This allows us to easily splice in a drm_bridge (which will register
the drm_connector itself) with very little work. And even if there's
not a full-blown bridge you might have different connectors and
things.

> Another solution is to create a simple connector with modes:
>
> struct drm_simple_connector {
>         struct drm_connector connector;
>         const struct drm_display_mode *modes;
>         unsigned int num_modes;
> };
>
> struct drm_connector *drm_simple_connector_create(struct drm_device *dev,
>         const struct drm_display_mode *modes, unsigned int num_modes,
>         int connector_type);

Yeah, this makes more sense to me, but then we're kinda reinventing
something like simple-panel.c with this. Otoh right now with
smple_display_pipe it's not possible to wire up the panel callbacks
easily, so maybe it should be a drm_bridge. Or we just leave this code
in tinydrm and extract it when someone else has a need for it?
-Daniel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch