[PATCH weston v3] drm: port the drm backend to the new init api
Benoit Gschwind
gschwind at gnu-log.net
Fri Feb 5 20:03:20 UTC 2016
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:
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>
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.
Best regards.
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
>
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list