[PATCH weston v3] drm: port the drm backend to the new init api
Pekka Paalanen
ppaalanen at gmail.com
Mon Feb 8 10:02:20 UTC 2016
On Fri, 5 Feb 2016 21:03:20 +0100
Benoit Gschwind <gschwind at gnu-log.net> wrote:
> Hello,
>
> I will add my opinion as called for opinions. First I made a quick
> brainstorm following previous proposals about solutions available. I see
> 3 mains choice:
Hi,
thanks for looking into this.
> 1. a structure that user fill and pass to the back end;
> 2. an opaque structure that the user fill through helper function;
> 3. a free list of key/value pair.
>
> For the first :
> - user know what must be set up,
> - not forward compatibility,
> - low level>
It can be backward compatible if the structs are versioned. A new version
of libweston can add new fields to the end of the struct, and the
version will tell it whether those fields are actually present. This
way libweston can add more options without breaking old compositors.
It is also possible for compositors to check libweston version at
runtime, and pick an older struct version based on that at runtime,
because the part of the struct that actually ends up used will be
identical regardless of the header version used to build it.
> For the second:
> - can have a hidden version,
> - can use initializer to know which value have been set or not (as
> replacement of version),
> - have to define a set of helper functions to fill the structure,
> - "type safe".
>
> For the third:
> - can use free string as key or predefined enums,
> - have a free number of parameters,
> - can have multiple times the same key (depending on how it's
> implemented),
> - have a more complex memory "structure",
> - value are not type safe without care.
>
> My personal preference go for the second one, it's a good trade off. And
> I suggest to have hidden parameters to check that each parameters are
> properly filled by the user.
The downside with the second one is that we would need to implement all
the setter functions in Weston core. This means Weston core will grow
"lots" of backend-specific code - at least measured in number of
exported symbols.
But would it really be that bad?
It also means it will not be easy for a compositor to support new
libweston backend options conditionally at runtime. That is, build
against a newer libweston while retaining runtime compatibility against
an older libweston. However, is that important enough to be considered
(yet?), I cannot say.
The third case was inspired by EGL to have libweston ABI both forward
and backward compatible, FWIW, and to keep backend-specific functions
out of libweston core.
Each design has pros and cons, and at least to me it is not obvious yet
which one is superior in practice. At this time I would consider having
an implementation written to be a huge pro, so I'd be inclined to go
with that.
Thanks,
pq
> Le 14/12/2015 09:15, Pekka Paalanen a écrit :
> > 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: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160208/23abbcf6/attachment.sig>
More information about the wayland-devel
mailing list