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

[Pekka Paalanen]

On Tue, 24 Nov 2015 16:31:42 +0200
Giulio Camuffo  wrote:

> 2015-11-20 11:38 GMT+02:00 Quentin Glidic :
> > For now, I will just comment on the part I am not too happy with.
> >
> > On 31/10/2015 12:08, Giulio Camuffo wrote:  
> >>
> >> [snip]

> >> +static void
> >> +drm_configure_output(struct weston_compositor *c, const char *name,
> >> +struct weston_drm_backend_output_config *config)
> >> +{
> >> +   struct weston_config *wc = weston_compositor_get_user_data(c);
> >> +   struct weston_config_section *section;
> >> +   char *s;
> >> +   int scale;
> >> +
> >> +   section = weston_config_get_section(wc, "output", "name", name);
> >> +   weston_config_section_get_string(section, "mode", ,
> >> "preferred");
> >> +   if (strcmp(s, "off") == 0)
> >> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_OFF;
> >> +   else if (strcmp(s, "preferred") == 0)
> >> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_PREFERRED;
> >> +   else if (strcmp(s, "current") == 0)
> >> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_CURRENT;
> >> +   else if (sscanf(s, "%dx%d", >base.width,
> >> +   >base.height) == 2)
> >> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_MODE;
> >> +   else if (parse_modeline(s, >modeline) == 0)
> >> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_MODELINE;
> >> +   else {
> >> +   weston_log("Invalid mode \"%s\" for output %s\n",
> >> +  s, name);
> >> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_PREFERRED;
> >> +   }
> >> +   free(s);  
> >
> >
> > I would like this part shared between the backend and the compositor.
> > As I said, the parsing of modeline should be in the backend, but also what I
> > would call “light modeline”, i.e. "width x height". That put the “technical”
> > part into the “technical code”.
> > Then you have a separate setting for on/off(/current).
> >
> > The configure_output function would return a boolean, which indicates
> > whether or not to activate that output. The modeline would be passed as a
> > string (config->moduline) and could be NULL.
> > Returning FALSE obviously means that all the configuration will be ignored
> > (and thus, you can return early, keeping everything to NULL) and the meaning
> > of TRUE depends on the modeline:
> > - if it is a well-formed modeline (or “light modeline”), use it;
> > - if it is malformed, fallback to NULL;
> > - if it is NULL, use the preferred setting.
> >
> > If “current” is really a needed setting (I do not know the use case it was
> > added for), we can just use a 3-values enum as the return value:
> > - OFF = 0
> > - PREFERRED = 1
> > - CURRENT = -1
> > which will change the meaning of a NULL modeline.  
> 
> I'm not sure what current is for but i would not remove it.

Hi,

FWIW, these are the same as explained in weston.ini man page. Preferred
means the preferred mode in EDID or similar, while current means the
currently active video mode when the compositor is starting, to allow
avoiding a mode switch in case preferred != current. E.g. when you
force the mode on kernel command line.


Thanks,
pq


pgpDHWWi31n4j.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Pekka Paalanen]

On Mon, 8 Feb 2016 16:01:13 -0800
Bill Spitzak  wrote:

> I thought the purpose of this was so that compositors could pass to the
> backend DRM configuration data supplied by the clients, therefore the api
> that clients use to pass this information seemed pretty important.

No, this has absolutely nothing to do with clients or Wayland. That
would be wrong on so many levels.

Instead, this has to do with a third party compositor program that
links to a library called libweston, to pass backend-specific
configuration through libweston to the backend component. Backends are
a part of libweston.


Thanks,
pq

> On Mon, Feb 8, 2016 at 1:53 PM, Daniel Stone  wrote:
> 
> > On Monday, 8 February 2016, Bill Spitzak  wrote:
> >  
> >> On Mon, Feb 8, 2016 at 2:02 AM, Pekka Paalanen 
> >> wrote:
> >>  
> >>> On Fri, 5 Feb 2016 21:03:20 +0100
> >>> Benoit Gschwind  wrote:  
> >>> > 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.  
> >>>  
> >>
> >> The Wayland message api only allows sending of a small set of primitive
> >> data types. So if you have a "structure" that is more complex than one of
> >> those data types, the only way to send it is through multiple arguments to
> >> (possibly multiple) Wayland requests. Therefore it seems like it is limited
> >> to solutions 2 and 3, right?
> >>  
> >
> > ... you do know that libweston is a C library and not a Wayland protocol,
> > right?
> >
> > These are the sorts of things that make it difficult (trending towards
> > impossible) to take your review seriously.
> >  



pgpL_fFHJAF6_.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Daniel Stone]

On 9 February 2016 at 00:01, Bill Spitzak  wrote:
> I thought the purpose of this was so that compositors could pass to the
> backend DRM configuration data supplied by the clients, therefore the api
> that clients use to pass this information seemed pretty important.

If you actually read the patch before you started commenting on it,
you would realise this is not the case. But, as always, you extend us
the discourtesy of your ill-formed opinions on the first three lines
of something, without bothering to even read it, let alone actually
think about the implications.

Every time you post, you're wasting the time of 1127 subscribers
because you can't be bothered spending your own time to actually read
what you reply to. Sometimes you can make useful contributions, but
they're very much in the margins compared to the vast majority, where
you decide your time is 1127x more important than that of others.

*shakes head*
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bill Spitzak]

I thought the purpose of this was so that compositors could pass to the
backend DRM configuration data supplied by the clients, therefore the api
that clients use to pass this information seemed pretty important.


On Mon, Feb 8, 2016 at 1:53 PM, Daniel Stone  wrote:

> On Monday, 8 February 2016, Bill Spitzak  wrote:
>
>> On Mon, Feb 8, 2016 at 2:02 AM, Pekka Paalanen 
>> wrote:
>>
>>> On Fri, 5 Feb 2016 21:03:20 +0100
>>> Benoit Gschwind  wrote:
>>> > 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.
>>>
>>
>> The Wayland message api only allows sending of a small set of primitive
>> data types. So if you have a "structure" that is more complex than one of
>> those data types, the only way to send it is through multiple arguments to
>> (possibly multiple) Wayland requests. Therefore it seems like it is limited
>> to solutions 2 and 3, right?
>>
>
> ... you do know that libweston is a C library and not a Wayland protocol,
> right?
>
> These are the sorts of things that make it difficult (trending towards
> impossible) to take your review seriously.
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bill Spitzak]

On Mon, Feb 8, 2016 at 2:02 AM, Pekka Paalanen  wrote:

> On Fri, 5 Feb 2016 21:03:20 +0100
> Benoit Gschwind  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.
>

The Wayland message api only allows sending of a small set of primitive
data types. So if you have a "structure" that is more complex than one of
those data types, the only way to send it is through multiple arguments to
(possibly multiple) Wayland requests. Therefore it seems like it is limited
to solutions 2 and 3, right?

A list of key/value pairs has a huge advantage that intermediate layers do
not need to know anything about the data.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Pekka Paalanen]

On Tue, 9 Feb 2016 01:11:48 +0100
Benoit Gschwind  wrote:

> Hello,
> 
> while I developing the option 2. I raise few questions because this 
> option explicit the fact that back-ends does not share the same API (at 
> less for the configuration). To resolve the issue I found the following 
> options:
> 
>   1. find a common configuration API (use key/value)
> 
>   2. have the following split:
> - libweston: every thing that is not in back-ends.
> - libbackendx, libbackendy, ... : back end implementation of 
> configuration stuff, that you must link with libweston (this is the 
> uncommon API parts of back-ends
> - backendx.so, backendy.so, ... : the loadable (common API) part of 
> backends.
> 
>   3. have the following split:
>- libweston: every thing that is not in backends.
>- libbackendx, libbackendy, ... : back end are not more loadable, and 
> are linked at build, developer can drop useless back-ends.

Hi,

I understand option 2, but I do not understand how option 3 could work.

I suppose option 3 means that a compositor would be built to run just a
single backend, and you cannot choose it at runtime? We certainly do
not want to pose that kind of a limitation.

We also do not want to load more than one backend at a time to the
compositor process, because different backends pull in different
libraries and sometimes those are a significant burden. It would be sad
if a compositor that supported both DRM and X11 backends would *always*
load libX11 regardless of which it is using. It would be useless work
at start-up, and also make the dependencies of the compositor too much:
you cannot choose to install it with libs for DRM only. That would be
awkward for distributions.

About distributions, you can see the explanation on how Weston project
will be intended to be split into binary packages in a distribution in
Weston's readme.

> Maybe you have better idea, from my point of view I will prefer the 
> third one. it's not optimal in term of loading and memory because we 
> need to load all available back-ends but it's cleaner.

For the above reasons, I believe option 3 is not acceptable.

> Maybe in the option 3 we can hide the option 2 inside, that load a 
> backend.so only once it's used to save memory and loading time.
> 
> This is something I didn't foresee in my previous e-mail.

Right, so you propose we add yet another bunch of libraries, one per
backend, to be linked to by the compositor program at build time. The
benefit is that libweston.so would not export any backend-specific
symbols.

I think that is the only benefit, and the amount of code inside these
libraries would be quite small. Therefore to me the trade-off compared
to libweston.so just exporting those symbols itself is not worth it.

The summary in your previous email on the possible design choises is
still accurate in my opinion, this new idea here is just bringing more
complexity that it is worth.


Thanks,
pq


pgpy0Ropzh0oG.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Benoit Gschwind]

Le 09/02/2016 11:38, Pekka Paalanen a écrit :

On Tue, 9 Feb 2016 01:11:48 +0100
Benoit Gschwind  wrote:


Hello,

while I developing the option 2. I raise few questions because this
option explicit the fact that back-ends does not share the same API (at
less for the configuration). To resolve the issue I found the following
options:

   1. find a common configuration API (use key/value)

   2. have the following split:
 - libweston: every thing that is not in back-ends.
 - libbackendx, libbackendy, ... : back end implementation of
configuration stuff, that you must link with libweston (this is the
uncommon API parts of back-ends
 - backendx.so, backendy.so, ... : the loadable (common API) part of
backends.

   3. have the following split:
- libweston: every thing that is not in backends.
- libbackendx, libbackendy, ... : back end are not more loadable, and
are linked at build, developer can drop useless back-ends.


Hi,

I understand option 2, but I do not understand how option 3 could work.

I suppose option 3 means that a compositor would be built to run just a
single backend, and you cannot choose it at runtime? We certainly do
not want to pose that kind of a limitation.


no, I didn't thinked that design, you can have several backend in the 
option3 with :

 - banckend_handler * load_aaa_bakcend (provided by libbackendaaa)
 - banckend_handler * load_bbb_bakcend (provided by libbackendbbb)

you can link both and select the desired backend, I did this kind of 
thing in my proposed patch.




We also do not want to load more than one backend at a time to the
compositor process, because different backends pull in different
libraries and sometimes those are a significant burden. It would be sad
if a compositor that supported both DRM and X11 backends would *always*
load libX11 regardless of which it is using. It would be useless work
at start-up, and also make the dependencies of the compositor too much:
you cannot choose to install it with libs for DRM only. That would be
awkward for distributions.

About distributions, you can see the explanation on how Weston project
will be intended to be split into binary packages in a distribution in
Weston's readme.


Ok, I was not aware of this direction, now it's clear and I agree, I 
found a compromise between option 2 and 3, I leave you to check my 
submitted patch.





Maybe you have better idea, from my point of view I will prefer the
third one. it's not optimal in term of loading and memory because we
need to load all available back-ends but it's cleaner.


For the above reasons, I believe option 3 is not acceptable.


Maybe in the option 3 we can hide the option 2 inside, that load a
backend.so only once it's used to save memory and loading time.

This is something I didn't foresee in my previous e-mail.


Right, so you propose we add yet another bunch of libraries, one per
backend, to be linked to by the compositor program at build time. The
benefit is that libweston.so would not export any backend-specific
symbols.

I think that is the only benefit, and the amount of code inside these
libraries would be quite small. Therefore to me the trade-off compared
to libweston.so just exporting those symbols itself is not worth it.

The summary in your previous email on the possible design choises is
still accurate in my opinion, this new idea here is just bringing more
complexity that it is worth.


Thanks,
pq


___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Pekka Paalanen]

On Fri, 5 Feb 2016 21:03:20 +0100
Benoit Gschwind  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  wrote:
> >  
> >> 2015-12-11 17:31 GMT+02:00 Pekka Paalanen :  
> >>> On Wed, 9 Dec 2015 19:08:58 +0200
> >>> Giulio Camuffo  wrote:
> >>>  
>  2015-12-09 18:58 GMT+02:00 Daniel Stone :  
> > Hi,
> >
> > On 9 December 2015 at 16:29, Giulio Camuffo  
> > 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;
> >> +* - "xrgb";
> >> +* - "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.
> 

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

[Daniel Stone]

On Monday, 8 February 2016, Bill Spitzak  wrote:

> On Mon, Feb 8, 2016 at 2:02 AM, Pekka Paalanen  > wrote:
>
>> On Fri, 5 Feb 2016 21:03:20 +0100
>> Benoit Gschwind > > wrote:
>> > 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.
>>
>
> The Wayland message api only allows sending of a small set of primitive
> data types. So if you have a "structure" that is more complex than one of
> those data types, the only way to send it is through multiple arguments to
> (possibly multiple) Wayland requests. Therefore it seems like it is limited
> to solutions 2 and 3, right?
>

... you do know that libweston is a C library and not a Wayland protocol,
right?

These are the sorts of things that make it difficult (trending towards
impossible) to take your review seriously.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Benoit Gschwind]

Hello,

while I developing the option 2. I raise few questions because this 
option explicit the fact that back-ends does not share the same API (at 
less for the configuration). To resolve the issue I found the following 
options:


 1. find a common configuration API (use key/value)

 2. have the following split:
   - libweston: every thing that is not in back-ends.
   - libbackendx, libbackendy, ... : back end implementation of 
configuration stuff, that you must link with libweston (this is the 
uncommon API parts of back-ends
   - backendx.so, backendy.so, ... : the loadable (common API) part of 
backends.


 3. have the following split:
  - libweston: every thing that is not in backends.
  - libbackendx, libbackendy, ... : back end are not more loadable, and 
are linked at build, developer can drop useless back-ends.



Maybe you have better idea, from my point of view I will prefer the 
third one. it's not optimal in term of loading and memory because we 
need to load all available back-ends but it's cleaner.


Maybe in the option 3 we can hide the option 2 inside, that load a 
backend.so only once it's used to save memory and loading time.


This is something I didn't foresee in my previous e-mail.

Best regards.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Benoit Gschwind]

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  wrote:


2015-12-11 17:31 GMT+02:00 Pekka Paalanen :

On Wed, 9 Dec 2015 19:08:58 +0200
Giulio Camuffo  wrote:


2015-12-09 18:58 GMT+02:00 Daniel Stone :

Hi,

On 9 December 2015 at 16:29, Giulio Camuffo  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;
+* - "xrgb";
+* - "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 ("xrgb") will be used;
+* - "xrgb";
+* - "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 

Re: [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  wrote:

> 2015-12-11 17:31 GMT+02:00 Pekka Paalanen :
> > On Wed, 9 Dec 2015 19:08:58 +0200
> > Giulio Camuffo  wrote:
> >  
> >> 2015-12-09 18:58 GMT+02:00 Daniel Stone :  
> >> > Hi,
> >> >
> >> > On 9 December 2015 at 16:29, Giulio Camuffo  
> >> > 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;
> >> >> +* - "xrgb";
> >> >> +* - "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 ("xrgb") will be used;
> >> >> +* - "xrgb";
> >> >> +* - "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.

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

[Pekka Paalanen]

On Wed, 9 Dec 2015 19:08:58 +0200
Giulio Camuffo  wrote:

> 2015-12-09 18:58 GMT+02:00 Daniel Stone :
> > Hi,
> >
> > On 9 December 2015 at 16:29, Giulio Camuffo  
> > 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;
> >> +* - "xrgb";
> >> +* - "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 ("xrgb") will be used;
> >> +* - "xrgb";
> >> +* - "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.

The good thing about structs is that they are type-safe 

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

[Giulio Camuffo]

Reviewed-by: Quentin Glidic 
---

v3: - move the modeline parsing back in the drm code, so that compositors
  can rely on that instead of having to duplicate the parsing code.
- add documentation in compositor-drm.h

 Makefile.am  |   3 +
 src/compositor-drm.c | 195 ++-
 src/compositor-drm.h | 111 +
 src/compositor.h |   2 -
 src/main.c   |  94 -
 5 files changed, 275 insertions(+), 130 deletions(-)
 create mode 100644 src/compositor-drm.h

diff --git a/Makefile.am b/Makefile.am
index cbb3b57..ff43531 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -72,6 +72,7 @@ weston_SOURCES =  \
src/log.c   \
src/compositor.c\
src/compositor.h\
+   src/compositor-drm.h\
src/input.c \
src/data-device.c   \
src/screenshooter.c \
@@ -207,6 +208,7 @@ westonincludedir = $(includedir)/weston
 westoninclude_HEADERS =\
src/version.h   \
src/compositor.h\
+   src/compositor-drm.h\
src/timeline-object.h   \
shared/matrix.h \
shared/config-parser.h  \
@@ -276,6 +278,7 @@ drm_backend_la_CFLAGS = \
$(AM_CFLAGS)
 drm_backend_la_SOURCES =   \
src/compositor-drm.c\
+   src/compositor-drm.h\
$(INPUT_BACKEND_SOURCES)\
shared/helpers.h\
shared/timespec-util.h  \
diff --git a/src/compositor-drm.c b/src/compositor-drm.c
index ea6f3cd..66352f1 100644
--- a/src/compositor-drm.c
+++ b/src/compositor-drm.c
@@ -50,6 +50,7 @@
 #include "shared/timespec-util.h"
 #include "libbacklight.h"
 #include "compositor.h"
+#include "compositor-drm.h"
 #include "gl-renderer.h"
 #include "pixman-renderer.h"
 #include "libinput-seat.h"
@@ -74,17 +75,6 @@
 #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
 #endif
 
-static int option_current_mode = 0;
-
-enum output_config {
-   OUTPUT_CONFIG_INVALID = 0,
-   OUTPUT_CONFIG_OFF,
-   OUTPUT_CONFIG_PREFERRED,
-   OUTPUT_CONFIG_CURRENT,
-   OUTPUT_CONFIG_MODE,
-   OUTPUT_CONFIG_MODELINE
-};
-
 struct drm_backend {
struct weston_backend base;
struct weston_compositor *compositor;
@@ -130,6 +120,8 @@ struct drm_backend {
 
int32_t cursor_width;
int32_t cursor_height;
+
+   struct weston_drm_backend_config *config;
 };
 
 struct drm_mode {
@@ -220,13 +212,6 @@ struct drm_sprite {
uint32_t formats[];
 };
 
-struct drm_parameters {
-   int connector;
-   int tty;
-   int use_pixman;
-   const char *seat_id;
-};
-
 static struct gl_renderer_interface *gl_renderer;
 
 static const char default_seat[] = "seat0";
@@ -2143,16 +2128,10 @@ setup_output_seat_constraint(struct drm_backend *b,
 }
 
 static int
-get_gbm_format_from_section(struct weston_config_section *section,
-   uint32_t default_value,
-   uint32_t *format)
+parse_gbm_format(const char *s, uint32_t default_value, uint32_t *format)
 {
-   char *s;
int ret = 0;
 
-   weston_config_section_get_string(section,
-"gbm-format", , NULL);
-
if (s == NULL)
*format = default_value;
else if (strcmp(s, "xrgb") == 0)
@@ -2166,8 +2145,6 @@ get_gbm_format_from_section(struct weston_config_section 
*section,
ret = -1;
}
 
-   free(s);
-
return ret;
 }
 
@@ -2177,30 +2154,44 @@ get_gbm_format_from_section(struct 
weston_config_section *section,
  * Find the most suitable mode to use for initial setup (or reconfiguration on
  * hotplug etc) for a DRM output.
  *
+ * @param backend The DRM backend object
  * @param output DRM output to choose mode for
- * @param kind Strategy and preference to use when choosing mode
- * @param width Desired width for this output
- * @param height Desired height for this output
+ * @param config Desired configuration for the output
  * @param current_mode Mode currently being displayed on this output
- * @param modeline Manually-entered mode (may be NULL)
  * @returns A mode from the output's mode list, or NULL if none available
  */
 static struct drm_mode *
-drm_output_choose_initial_mode(struct drm_output *output,
-  enum output_config kind,
-  int width, int height,
-  

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

[Giulio Camuffo]

I forgot to mention, this requires a revert of
5ffbfffaf7758c33791978516d0a1100773b85e2.

--
Giulio

2015-12-09 18:29 GMT+02:00 Giulio Camuffo :
> Reviewed-by: Quentin Glidic 
> ---
>
> v3: - move the modeline parsing back in the drm code, so that compositors
>   can rely on that instead of having to duplicate the parsing code.
> - add documentation in compositor-drm.h
>
>  Makefile.am  |   3 +
>  src/compositor-drm.c | 195 
> ++-
>  src/compositor-drm.h | 111 +
>  src/compositor.h |   2 -
>  src/main.c   |  94 -
>  5 files changed, 275 insertions(+), 130 deletions(-)
>  create mode 100644 src/compositor-drm.h
>
> diff --git a/Makefile.am b/Makefile.am
> index cbb3b57..ff43531 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -72,6 +72,7 @@ weston_SOURCES =  \
> src/log.c   \
> src/compositor.c\
> src/compositor.h\
> +   src/compositor-drm.h\
> src/input.c \
> src/data-device.c   \
> src/screenshooter.c \
> @@ -207,6 +208,7 @@ westonincludedir = $(includedir)/weston
>  westoninclude_HEADERS =\
> src/version.h   \
> src/compositor.h\
> +   src/compositor-drm.h\
> src/timeline-object.h   \
> shared/matrix.h \
> shared/config-parser.h  \
> @@ -276,6 +278,7 @@ drm_backend_la_CFLAGS = \
> $(AM_CFLAGS)
>  drm_backend_la_SOURCES =   \
> src/compositor-drm.c\
> +   src/compositor-drm.h\
> $(INPUT_BACKEND_SOURCES)\
> shared/helpers.h\
> shared/timespec-util.h  \
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index ea6f3cd..66352f1 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -50,6 +50,7 @@
>  #include "shared/timespec-util.h"
>  #include "libbacklight.h"
>  #include "compositor.h"
> +#include "compositor-drm.h"
>  #include "gl-renderer.h"
>  #include "pixman-renderer.h"
>  #include "libinput-seat.h"
> @@ -74,17 +75,6 @@
>  #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
>  #endif
>
> -static int option_current_mode = 0;
> -
> -enum output_config {
> -   OUTPUT_CONFIG_INVALID = 0,
> -   OUTPUT_CONFIG_OFF,
> -   OUTPUT_CONFIG_PREFERRED,
> -   OUTPUT_CONFIG_CURRENT,
> -   OUTPUT_CONFIG_MODE,
> -   OUTPUT_CONFIG_MODELINE
> -};
> -
>  struct drm_backend {
> struct weston_backend base;
> struct weston_compositor *compositor;
> @@ -130,6 +120,8 @@ struct drm_backend {
>
> int32_t cursor_width;
> int32_t cursor_height;
> +
> +   struct weston_drm_backend_config *config;
>  };
>
>  struct drm_mode {
> @@ -220,13 +212,6 @@ struct drm_sprite {
> uint32_t formats[];
>  };
>
> -struct drm_parameters {
> -   int connector;
> -   int tty;
> -   int use_pixman;
> -   const char *seat_id;
> -};
> -
>  static struct gl_renderer_interface *gl_renderer;
>
>  static const char default_seat[] = "seat0";
> @@ -2143,16 +2128,10 @@ setup_output_seat_constraint(struct drm_backend *b,
>  }
>
>  static int
> -get_gbm_format_from_section(struct weston_config_section *section,
> -   uint32_t default_value,
> -   uint32_t *format)
> +parse_gbm_format(const char *s, uint32_t default_value, uint32_t *format)
>  {
> -   char *s;
> int ret = 0;
>
> -   weston_config_section_get_string(section,
> -"gbm-format", , NULL);
> -
> if (s == NULL)
> *format = default_value;
> else if (strcmp(s, "xrgb") == 0)
> @@ -2166,8 +2145,6 @@ get_gbm_format_from_section(struct 
> weston_config_section *section,
> ret = -1;
> }
>
> -   free(s);
> -
> return ret;
>  }
>
> @@ -2177,30 +2154,44 @@ get_gbm_format_from_section(struct 
> weston_config_section *section,
>   * Find the most suitable mode to use for initial setup (or reconfiguration 
> on
>   * hotplug etc) for a DRM output.
>   *
> + * @param backend The DRM backend object
>   * @param output DRM output to choose mode for
> - * @param kind Strategy and preference to use when choosing mode
> - * @param width Desired width for this output
> - * @param height Desired height for this output
> + * @param config Desired configuration for the output
>   * 

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

[Giulio Camuffo]

2015-12-09 18:58 GMT+02:00 Daniel Stone :
> Hi,
>
> On 9 December 2015 at 16:29, Giulio Camuffo  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;
>> +* - "xrgb";
>> +* - "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 ("xrgb") will be used;
>> +* - "xrgb";
>> +* - "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. ;)
>
> Cheers,
> Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Daniel Stone]

Hi,

On 9 December 2015 at 16:29, Giulio Camuffo  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;
> +* - "xrgb";
> +* - "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 ("xrgb") will be used;
> +* - "xrgb";
> +* - "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.

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. ;)

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Giulio Camuffo]

2015-11-20 11:38 GMT+02:00 Quentin Glidic :
> For now, I will just comment on the part I am not too happy with.
>
> On 31/10/2015 12:08, Giulio Camuffo wrote:
>>
>> [snip]
>>
>> diff --git a/src/main.c b/src/main.c
>> index 292f8e0..bde27ee 100644
>> --- a/src/main.c
>> +++ b/src/main.c
>> @@ -47,6 +47,8 @@
>>   #include "git-version.h"
>>   #include "version.h"
>>
>> +#include "compositor-drm.h"
>> +
>>   static struct wl_list child_process_list;
>>   static struct weston_compositor *segv_compositor;
>>
>> @@ -666,12 +668,137 @@ load_backend_new(struct weston_compositor
>> *compositor, const char *backend,
>>   }
>>
>>   static int
>> +parse_modeline(const char *s, struct weston_drm_backend_modeline *mode)
>> +{
>> +   char hsync[16];
>> +   char vsync[16];
>> +   float fclock;
>> +
>> +   mode->flags = 0;
>> +
>> +   if (sscanf(s, "%f %hd %hd %hd %hd %hd %hd %hd %hd %15s %15s",
>> +  ,
>> +  >hdisplay,
>> +  >hsync_start,
>> +  >hsync_end,
>> +  >htotal,
>> +  >vdisplay,
>> +  >vsync_start,
>> +  >vsync_end,
>> +  >vtotal, hsync, vsync) != 11)
>> +   return -1;
>> +
>> +   mode->clock = fclock * 1000;
>> +   if (strcmp(hsync, "+hsync") == 0)
>> +   mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_PHSYNC;
>> +   else if (strcmp(hsync, "-hsync") == 0)
>> +   mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_NHSYNC;
>> +   else
>> +   return -1;
>> +
>> +   if (strcmp(vsync, "+vsync") == 0)
>> +   mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_PVSYNC;
>> +   else if (strcmp(vsync, "-vsync") == 0)
>> +   mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_NVSYNC;
>> +   else
>> +   return -1;
>> +
>> +   return 0;
>> +}
>
>
> As discussed on IRC, I think this part should stay in the drm backend. The
> rationale is that we probably want all libweston-based compositors to work
> with the exact same modeline format, with the same error messages and the
> same predictable behaviour.
>
>
>
>> +static void
>> +drm_configure_output(struct weston_compositor *c, const char *name,
>> +struct weston_drm_backend_output_config *config)
>> +{
>> +   struct weston_config *wc = weston_compositor_get_user_data(c);
>> +   struct weston_config_section *section;
>> +   char *s;
>> +   int scale;
>> +
>> +   section = weston_config_get_section(wc, "output", "name", name);
>> +   weston_config_section_get_string(section, "mode", ,
>> "preferred");
>> +   if (strcmp(s, "off") == 0)
>> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_OFF;
>> +   else if (strcmp(s, "preferred") == 0)
>> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_PREFERRED;
>> +   else if (strcmp(s, "current") == 0)
>> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_CURRENT;
>> +   else if (sscanf(s, "%dx%d", >base.width,
>> +   >base.height) == 2)
>> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_MODE;
>> +   else if (parse_modeline(s, >modeline) == 0)
>> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_MODELINE;
>> +   else {
>> +   weston_log("Invalid mode \"%s\" for output %s\n",
>> +  s, name);
>> +   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_PREFERRED;
>> +   }
>> +   free(s);
>
>
> I would like this part shared between the backend and the compositor.
> As I said, the parsing of modeline should be in the backend, but also what I
> would call “light modeline”, i.e. "width x height". That put the “technical”
> part into the “technical code”.
> Then you have a separate setting for on/off(/current).
>
> The configure_output function would return a boolean, which indicates
> whether or not to activate that output. The modeline would be passed as a
> string (config->moduline) and could be NULL.
> Returning FALSE obviously means that all the configuration will be ignored
> (and thus, you can return early, keeping everything to NULL) and the meaning
> of TRUE depends on the modeline:
> - if it is a well-formed modeline (or “light modeline”), use it;
> - if it is malformed, fallback to NULL;
> - if it is NULL, use the preferred setting.
>
> If “current” is really a needed setting (I do not know the use case it was
> added for), we can just use a 3-values enum as the return value:
> - OFF = 0
> - PREFERRED = 1
> - CURRENT = -1
> which will change the meaning of a NULL modeline.

I'm not sure what current is for but i would not remove it.

>
>
> What do you think?
> If we agree on that, I can make a patch for this (as a follow-up to squash
> before the push maybe).

I think it sounds ok, but i would have to see how it turns up to be sure ;)

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

[Quentin Glidic]

For now, I will just comment on the part I am not too happy with.

On 31/10/2015 12:08, Giulio Camuffo wrote:

[snip]
diff --git a/src/main.c b/src/main.c
index 292f8e0..bde27ee 100644
--- a/src/main.c
+++ b/src/main.c
@@ -47,6 +47,8 @@
  #include "git-version.h"
  #include "version.h"

+#include "compositor-drm.h"
+
  static struct wl_list child_process_list;
  static struct weston_compositor *segv_compositor;

@@ -666,12 +668,137 @@ load_backend_new(struct weston_compositor *compositor, 
const char *backend,
  }

  static int
+parse_modeline(const char *s, struct weston_drm_backend_modeline *mode)
+{
+   char hsync[16];
+   char vsync[16];
+   float fclock;
+
+   mode->flags = 0;
+
+   if (sscanf(s, "%f %hd %hd %hd %hd %hd %hd %hd %hd %15s %15s",
+  ,
+  >hdisplay,
+  >hsync_start,
+  >hsync_end,
+  >htotal,
+  >vdisplay,
+  >vsync_start,
+  >vsync_end,
+  >vtotal, hsync, vsync) != 11)
+   return -1;
+
+   mode->clock = fclock * 1000;
+   if (strcmp(hsync, "+hsync") == 0)
+   mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_PHSYNC;
+   else if (strcmp(hsync, "-hsync") == 0)
+   mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_NHSYNC;
+   else
+   return -1;
+
+   if (strcmp(vsync, "+vsync") == 0)
+   mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_PVSYNC;
+   else if (strcmp(vsync, "-vsync") == 0)
+   mode->flags |= WESTON_DRM_BACKEND_MODELINE_FLAG_NVSYNC;
+   else
+   return -1;
+
+   return 0;
+}


As discussed on IRC, I think this part should stay in the drm backend. 
The rationale is that we probably want all libweston-based compositors 
to work with the exact same modeline format, with the same error 
messages and the same predictable behaviour.




+static void
+drm_configure_output(struct weston_compositor *c, const char *name,
+struct weston_drm_backend_output_config *config)
+{
+   struct weston_config *wc = weston_compositor_get_user_data(c);
+   struct weston_config_section *section;
+   char *s;
+   int scale;
+
+   section = weston_config_get_section(wc, "output", "name", name);
+   weston_config_section_get_string(section, "mode", , "preferred");
+   if (strcmp(s, "off") == 0)
+   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_OFF;
+   else if (strcmp(s, "preferred") == 0)
+   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_PREFERRED;
+   else if (strcmp(s, "current") == 0)
+   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_CURRENT;
+   else if (sscanf(s, "%dx%d", >base.width,
+   >base.height) == 2)
+   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_MODE;
+   else if (parse_modeline(s, >modeline) == 0)
+   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_MODELINE;
+   else {
+   weston_log("Invalid mode \"%s\" for output %s\n",
+  s, name);
+   config->type = WESTON_DRM_BACKEND_OUTPUT_TYPE_PREFERRED;
+   }
+   free(s);


I would like this part shared between the backend and the compositor.
As I said, the parsing of modeline should be in the backend, but also 
what I would call “light modeline”, i.e. "width x height". That put the 
“technical” part into the “technical code”.

Then you have a separate setting for on/off(/current).

The configure_output function would return a boolean, which indicates 
whether or not to activate that output. The modeline would be passed as 
a string (config->moduline) and could be NULL.
Returning FALSE obviously means that all the configuration will be 
ignored (and thus, you can return early, keeping everything to NULL) and 
the meaning of TRUE depends on the modeline:

- if it is a well-formed modeline (or “light modeline”), use it;
- if it is malformed, fallback to NULL;
- if it is NULL, use the preferred setting.

If “current” is really a needed setting (I do not know the use case it 
was added for), we can just use a 3-values enum as the return value:

- OFF = 0
- PREFERRED = 1
- CURRENT = -1
which will change the meaning of a NULL modeline.


What do you think?
If we agree on that, I can make a patch for this (as a follow-up to 
squash before the push maybe).


Cheers,

--

Quentin “Sardem FF7” Glidic
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Giulio Camuffo]

---

v3: fixed warning after rebase

 Makefile.am  |   3 +
 src/compositor-drm.c | 234 ---
 src/compositor-drm.h |  87 +++
 src/main.c   | 129 +++-
 4 files changed, 292 insertions(+), 161 deletions(-)
 create mode 100644 src/compositor-drm.h

diff --git a/Makefile.am b/Makefile.am
index 55e3bcb..b2238ea 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -72,6 +72,7 @@ weston_SOURCES =  \
src/log.c   \
src/compositor.c\
src/compositor.h\
+   src/compositor-drm.h\
src/input.c \
src/data-device.c   \
src/screenshooter.c \
@@ -209,6 +210,7 @@ westonincludedir = $(includedir)/weston
 westoninclude_HEADERS =\
src/version.h   \
src/compositor.h\
+   src/compositor-drm.h\
src/timeline-object.h   \
shared/matrix.h \
shared/config-parser.h  \
@@ -278,6 +280,7 @@ drm_backend_la_CFLAGS = \
$(AM_CFLAGS)
 drm_backend_la_SOURCES =   \
src/compositor-drm.c\
+   src/compositor-drm.h\
$(INPUT_BACKEND_SOURCES)\
shared/helpers.h\
shared/timespec-util.h  \
diff --git a/src/compositor-drm.c b/src/compositor-drm.c
index 41f9a82..85e8b04 100644
--- a/src/compositor-drm.c
+++ b/src/compositor-drm.c
@@ -50,6 +50,7 @@
 #include "shared/timespec-util.h"
 #include "libbacklight.h"
 #include "compositor.h"
+#include "compositor-drm.h"
 #include "gl-renderer.h"
 #include "pixman-renderer.h"
 #include "libinput-seat.h"
@@ -74,17 +75,6 @@
 #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
 #endif
 
-static int option_current_mode = 0;
-
-enum output_config {
-   OUTPUT_CONFIG_INVALID = 0,
-   OUTPUT_CONFIG_OFF,
-   OUTPUT_CONFIG_PREFERRED,
-   OUTPUT_CONFIG_CURRENT,
-   OUTPUT_CONFIG_MODE,
-   OUTPUT_CONFIG_MODELINE
-};
-
 struct drm_backend {
struct weston_backend base;
struct weston_compositor *compositor;
@@ -107,6 +97,7 @@ struct drm_backend {
uint32_t connector_allocator;
struct wl_listener session_listener;
uint32_t format;
+   bool option_current_mode;
 
/* we need these parameters in order to not fail drmModeAddFB2()
 * due to out of bounds dimensions, and then mistakenly set
@@ -130,6 +121,10 @@ struct drm_backend {
 
int32_t cursor_width;
int32_t cursor_height;
+
+   void (*configure_output)(struct weston_compositor *compositor,
+const char *name,
+struct weston_drm_backend_output_config 
*config);
 };
 
 struct drm_mode {
@@ -220,13 +215,6 @@ struct drm_sprite {
uint32_t formats[];
 };
 
-struct drm_parameters {
-   int connector;
-   int tty;
-   int use_pixman;
-   const char *seat_id;
-};
-
 static struct gl_renderer_interface *gl_renderer;
 
 static const char default_seat[] = "seat0";
@@ -2066,47 +2054,26 @@ find_and_parse_output_edid(struct drm_backend *b,
 
 
 
-static int
-parse_modeline(const char *s, drmModeModeInfo *mode)
-{
-   char hsync[16];
-   char vsync[16];
-   float fclock;
-
-   mode->type = DRM_MODE_TYPE_USERDEF;
-   mode->hskew = 0;
-   mode->vscan = 0;
-   mode->vrefresh = 0;
-   mode->flags = 0;
-
-   if (sscanf(s, "%f %hd %hd %hd %hd %hd %hd %hd %hd %15s %15s",
-  ,
-  >hdisplay,
-  >hsync_start,
-  >hsync_end,
-  >htotal,
-  >vdisplay,
-  >vsync_start,
-  >vsync_end,
-  >vtotal, hsync, vsync) != 11)
-   return -1;
-
-   mode->clock = fclock * 1000;
-   if (strcmp(hsync, "+hsync") == 0)
-   mode->flags |= DRM_MODE_FLAG_PHSYNC;
-   else if (strcmp(hsync, "-hsync") == 0)
-   mode->flags |= DRM_MODE_FLAG_NHSYNC;
-   else
-   return -1;
+static void
+weston_modeline_to_drm(struct weston_drm_backend_modeline *modeline,
+  drmModeModeInfo *drm_mode)
+{
+   drm_mode->type = DRM_MODE_TYPE_USERDEF;
+   drm_mode->hskew = 0;
+   drm_mode->vscan = 0;
+   drm_mode->vrefresh = 0;
+   drm_mode->flags = modeline->flags;
+   drm_mode->clock = modeline->clock;
 
-   if (strcmp(vsync, "+vsync") == 0)
-   mode->flags |= DRM_MODE_FLAG_PVSYNC;
-   else