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 <[email protected]> wrote:
2015-12-11 17:31 GMT+02:00 Pekka Paalanen <[email protected]>:
On Wed, 9 Dec 2015 19:08:58 +0200
Giulio Camuffo <[email protected]> wrote:
2015-12-09 18:58 GMT+02:00 Daniel Stone <[email protected]>:
Hi,
On 9 December 2015 at 16:29, Giulio Camuffo <[email protected]> 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 <[email protected]>
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
