[PATCH weston v3] drm: port the drm backend to the new init api

[Pekka Paalanen]

On Fri, 11 Dec 2015 17:49:57 +0200
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> 2015-12-11 17:31 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> > On Wed, 9 Dec 2015 19:08:58 +0200
> > Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> >  
> >> 2015-12-09 18:58 GMT+02:00 Daniel Stone <daniel at fooishbar.org>:  
> >> > Hi,
> >> >
> >> > On 9 December 2015 at 16:29, Giulio Camuffo <giuliocamuffo at gmail.com> wrote:  
> >> >> +enum weston_drm_backend_output_mode {
> >> >> +       /** The output is disabled */
> >> >> +       WESTON_DRM_BACKEND_OUTPUT_OFF,
> >> >> +       /** The output will use the current active mode */
> >> >> +       WESTON_DRM_BACKEND_OUTPUT_CURRENT,
> >> >> +       /** The output will use the preferred mode. A modeline can be provided
> >> >> +        * by setting weston_backend_output_config::modeline in the form of
> >> >> +        * "WIDTHxHEIGHT" or in the form of an explicit modeline calculated
> >> >> +        * using e.g. the cvt tool. If a valid modeline is supplied it will be
> >> >> +        * used, if invalid or NULL the preferred available mode will be used. */
> >> >> +       WESTON_DRM_BACKEND_OUTPUT_PREFERRED,
> >> >> +};
> >> >> +
> >> >> +struct weston_drm_backend_output_config {
> >> >> +       struct weston_backend_output_config base;
> >> >> +
> >> >> +       /** The pixel format to be used by the output. Valid values are:
> >> >> +        * - NULL - The format set at backend creation time will be used;
> >> >> +        * - "xrgb8888";
> >> >> +        * - "rgb565"
> >> >> +        * - "xrgb2101010"
> >> >> +        */
> >> >> +       char *format;
> >> >> +       /** The seat to be used by the output. Set to NULL to use the
> >> >> +        * default seat. */
> >> >> +       char *seat;
> >> >> +       /** The modeline to be used by the output. Refer to the documentation
> >> >> +        * of WESTON_DRM_BACKEND_OUTPUT_PREFERRED for details. */
> >> >> +       char *modeline;
> >> >> +};
> >> >> +
> >> >> +/** The backend configuration struct.
> >> >> + *
> >> >> + * weston_drm_backend_config contains the configuration used by a DRM
> >> >> + * backend. The backend will take ownership of the weston_backend_config
> >> >> + * object passed to it on initialization and will free it on destruction. */
> >> >> +struct weston_drm_backend_config {
> >> >> +       struct weston_backend_config base;
> >> >> +
> >> >> +       /** The connector id of the output to be initialized. A value of 0 will
> >> >> +        * enable all available outputs. */
> >> >> +       int connector;
> >> >> +       /** The tty to be used. Set to 0 to use the current tty. */
> >> >> +       int tty;
> >> >> +       /** If true the pixman renderer will be used instead of the OpenGL ES
> >> >> +        * renderer. */
> >> >> +       bool use_pixman;
> >> >> +       /** The seat to be used for input and output. If NULL the default "seat0"
> >> >> +        * will be used.
> >> >> +        * The backend will take ownership of the seat_id pointer and will free
> >> >> +        * it on backend destruction. */
> >> >> +       char *seat_id;
> >> >> +       /** The pixel format of the framebuffer to be used. Valid values are:
> >> >> +        * - NULL - The default format ("xrgb8888") will be used;
> >> >> +        * - "xrgb8888";
> >> >> +        * - "rgb565"
> >> >> +        * - "xrgb2101010"
> >> >> +        * The backend will take ownership of the format pointer and will free
> >> >> +        * it on backend destruction. */
> >> >> +       char *format;
> >> >> +
> >> >> +       /** Callback used to configure the outputs. This function will be called
> >> >> +        * by the backend when a new DRM output needs to be configured. */
> >> >> +       enum weston_drm_backend_output_mode
> >> >> +               (*configure_output)(struct weston_compositor *compositor,
> >> >> +                                   struct weston_drm_backend_config *backend_config,
> >> >> +                                   const char *name,
> >> >> +                                   struct weston_drm_backend_output_config *output_config);
> >> >> +};  
> >> >
> >> > My main concern here is that encoding this as ABI (every single option
> >> > for every single backend) seems a bit ambitious, and likely to lead to
> >> > version churn. To avoid this, you could look at either some kind of
> >> > generic key/value-store query API, or at least encoding a version
> >> > number into the struct, so you don't have to incompatibly break ABI
> >> > every time you add an option.  
> >>
> >> Well, but the plan is indeed to make multiple versions co-installable,
> >> and anyway the API/ABI will change in every weston release at least in
> >> the core so i don't think there is much point in going the extra mile
> >> to make the ABI stable here, just yet.
> >>  
> >> >
> >> > Other than that, looks fine enough to me, at a first pass, whilst
> >> > being sick. So probably not quite strong enough for a Reviewed-by. ;)  
> >
> > Hi,
> >
> > yeah. I recall I advocated this struct ABI approach as the first
> > attempt in just getting libweston out the door the first time. Daniel
> > is right, but I wonder how much it matters at this point. Of course, it
> > would be nice that new additions are not something we already know
> > we're going to have to rewrite.

...

> > And the it would be used in main.c like:
> >
> > struct backend_options_list *opts;
> >
> > opts = create_backend_option_list(5);
> > drm_add_option_connector(opts, 3);
> > drm_add_option_connector(opts, 4);
> > drm_add_option_seat(opts, "whee");
> >
> > load_backend_new(compositor, "drm-backend.so", opts);
> >
> >
> > How's that for an idea? I admit I didn't think it all through.  
> 
> I haven't really looked that carefully at it, but i must say i'm quite
> tired of changing this patch over and over. I am willing to change it
> again only if everybody relevant says that he's ok with the proposal,
> otherwise it's just too much work for maybe no gain. Even better would
> be to merge this for now, and to do a follow up later if we really
> don't like the structs.

Hi,

like I said below, I'm ok landing your design. I am also almost certain
it will need to be rewritten later.

So if we go with this design now, all backends need to get migrated to
this, and then later we'll migrate them all again. That's why I think
bikeshedding the drm-backend patch into shape according to our best
current knowledge now would be less work in total in the foreseeable
future.

But yes, I too would appreciate more opinions like you are calling
for. My proposal is just what came to my mind one day, it's just an
idea and not a requirement.

> > I didn't yet have time to properly review this patch, but regardless of
> > the ABI questions I am willing to give it at least:
> > Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Oh, and there's a compiler warning:
> >
> > /home/pq/git/weston/src/compositor-drm.c: In function ‘create_output_for_connector’:
> > /home/pq/git/weston/src/compositor-drm.c:2283:9: warning: missing braces around initializer [-Wmissing-braces]
> >   struct weston_drm_backend_output_config config = { 0 };
> >

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151214/e6e113ff/attachment.sig>