Re: [PATCH weston v5 01/36] libweston: introduce weston_head

2018-02-02 Thread Derek Foreman 

On 2018-02-02 02:06 AM, Pekka Paalanen wrote:

On Thu, 1 Feb 2018 13:59:24 -0600
Derek Foreman  wrote:


On 2017-12-14 05:40 AM, Pekka Paalanen wrote:

From: Pekka Paalanen 

In order to support clone modes, libweston needs the concept of a head
that is separate from weston_output. While weston_output manages buffers
and the repaint state machine, weston_head will represent a single
monitor. In the future it will be possible to have a single
weston_output drive one or more weston_heads for a clone mode that
shares the framebuffers between all cloned heads.

All the fields that are obviously properties of the monitor are moved
from weston_output into weston_head.

As moving the fields requires one to touch all the backends for all the
assingments, introduce setter functions for them while we are here. The
setters are identical to the old assignments, for now.

As a temporary measure, weston_output embeds a single head. Also the
ugly casts in weston_head_set_monitor_strings() will be removed by a
follow-up patch.

Signed-off-by: Pekka Paalanen 
---
   compositor/cms-colord.c |  32 ++--
   libweston/compositor-drm.c  |  15 +++---
   libweston/compositor-fbdev.c|  12 +++--
   libweston/compositor-headless.c |   8 +--
   libweston/compositor-rdp.c  |   8 +--
   libweston/compositor-wayland.c  |  18 ---
   libweston/compositor-x11.c  |  13 +++--
   libweston/compositor.c  | 105 
+++-
   libweston/compositor.h  |  38 +--
   9 files changed, 184 insertions(+), 65 deletions(-)




   void
+weston_head_set_monitor_strings(struct weston_head *head,
+   const char *make,
+   const char *model,
+   const char *serialno);


I was going to complain about the apparently silly trivial helper
functions, but their importance becomes obvious later in the series.


Right, and another reason is to pave way for making structs opaque, as
will be necessary for cleaning up the libweston ABI. Well, the setters
are for backends which would be fine having the struct definitions
visible, but I think it fits anyway.

Moving things towards a more proper libweston ABI is an idea carried
throughout the patch series.


Reviewed-by: Derek Foreman 
(though I don't want to see any of it land until we can get as far as
removing the embedded weston_head in weston_output...)


Very good. We need to get the atomic series landed then, before I can
rebase this series and include the drm-backend migration which is
required for removing the embedded head.


I'm vacillating on this point.  Atomic has seen so many revisions and 
delays that at this point making something dependent on it landing first 
seems unusually cruel.


weston_head being embedded in weston_output isn't even all that ugly as 
a refactor, ignoring it as a requisite step to get to clone mode.


I guess we'll consider this again in a few days.

Thanks,
Derek



Thanks,
pq



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

Re: [PATCH weston v5 01/36] libweston: introduce weston_head

2018-02-02 Thread Pekka Paalanen 
On Thu, 1 Feb 2018 13:59:24 -0600
Derek Foreman  wrote:

> On 2017-12-14 05:40 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen 
> > 
> > In order to support clone modes, libweston needs the concept of a head
> > that is separate from weston_output. While weston_output manages buffers
> > and the repaint state machine, weston_head will represent a single
> > monitor. In the future it will be possible to have a single
> > weston_output drive one or more weston_heads for a clone mode that
> > shares the framebuffers between all cloned heads.
> > 
> > All the fields that are obviously properties of the monitor are moved
> > from weston_output into weston_head.
> > 
> > As moving the fields requires one to touch all the backends for all the
> > assingments, introduce setter functions for them while we are here. The
> > setters are identical to the old assignments, for now.
> > 
> > As a temporary measure, weston_output embeds a single head. Also the
> > ugly casts in weston_head_set_monitor_strings() will be removed by a
> > follow-up patch.
> > 
> > Signed-off-by: Pekka Paalanen 
> > ---
> >   compositor/cms-colord.c |  32 ++--
> >   libweston/compositor-drm.c  |  15 +++---
> >   libweston/compositor-fbdev.c|  12 +++--
> >   libweston/compositor-headless.c |   8 +--
> >   libweston/compositor-rdp.c  |   8 +--
> >   libweston/compositor-wayland.c  |  18 ---
> >   libweston/compositor-x11.c  |  13 +++--
> >   libweston/compositor.c  | 105 
> > +++-
> >   libweston/compositor.h  |  38 +--
> >   9 files changed, 184 insertions(+), 65 deletions(-)
> > 

> >   void
> > +weston_head_set_monitor_strings(struct weston_head *head,
> > +   const char *make,
> > +   const char *model,
> > +   const char *serialno);  
> 
> I was going to complain about the apparently silly trivial helper 
> functions, but their importance becomes obvious later in the series.

Right, and another reason is to pave way for making structs opaque, as
will be necessary for cleaning up the libweston ABI. Well, the setters
are for backends which would be fine having the struct definitions
visible, but I think it fits anyway.

Moving things towards a more proper libweston ABI is an idea carried
throughout the patch series.

> Reviewed-by: Derek Foreman 
> (though I don't want to see any of it land until we can get as far as 
> removing the embedded weston_head in weston_output...)

Very good. We need to get the atomic series landed then, before I can
rebase this series and include the drm-backend migration which is
required for removing the embedded head.


Thanks,
pq


pgpggMITfjGZb.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 v5 01/36] libweston: introduce weston_head

2018-02-01 Thread Derek Foreman 

On 2017-12-14 05:40 AM, Pekka Paalanen wrote:

From: Pekka Paalanen 

In order to support clone modes, libweston needs the concept of a head
that is separate from weston_output. While weston_output manages buffers
and the repaint state machine, weston_head will represent a single
monitor. In the future it will be possible to have a single
weston_output drive one or more weston_heads for a clone mode that
shares the framebuffers between all cloned heads.

All the fields that are obviously properties of the monitor are moved
from weston_output into weston_head.

As moving the fields requires one to touch all the backends for all the
assingments, introduce setter functions for them while we are here. The
setters are identical to the old assignments, for now.

As a temporary measure, weston_output embeds a single head. Also the
ugly casts in weston_head_set_monitor_strings() will be removed by a
follow-up patch.

Signed-off-by: Pekka Paalanen 
---
  compositor/cms-colord.c |  32 ++--
  libweston/compositor-drm.c  |  15 +++---
  libweston/compositor-fbdev.c|  12 +++--
  libweston/compositor-headless.c |   8 +--
  libweston/compositor-rdp.c  |   8 +--
  libweston/compositor-wayland.c  |  18 ---
  libweston/compositor-x11.c  |  13 +++--
  libweston/compositor.c  | 105 +++-
  libweston/compositor.h  |  38 +--
  9 files changed, 184 insertions(+), 65 deletions(-)

diff --git a/compositor/cms-colord.c b/compositor/cms-colord.c
index 0daa2a7e..f421773b 100644
--- a/compositor/cms-colord.c
+++ b/compositor/cms-colord.c
@@ -102,22 +102,23 @@ edid_value_valid(const char *str)
  static gchar *
  get_output_id(struct cms_colord *cms, struct weston_output *o)
  {
+   struct weston_head *head = >head;
const gchar *tmp;
GString *device_id;
  
  	/* see https://github.com/hughsie/colord/blob/master/doc/device-and-profile-naming-spec.txt

 * for format and allowed values */
device_id = g_string_new("xrandr");
-   if (edid_value_valid(o->make)) {
-   tmp = g_hash_table_lookup(cms->pnp_ids, o->make);
+   if (edid_value_valid(head->make)) {
+   tmp = g_hash_table_lookup(cms->pnp_ids, head->make);
if (tmp == NULL)
-   tmp = o->make;
+   tmp = head->make;
g_string_append_printf(device_id, "-%s", tmp);
}
-   if (edid_value_valid(o->model))
-   g_string_append_printf(device_id, "-%s", o->model);
-   if (edid_value_valid(o->serial_number))
-   g_string_append_printf(device_id, "-%s", o->serial_number);
+   if (edid_value_valid(head->model))
+   g_string_append_printf(device_id, "-%s", head->model);
+   if (edid_value_valid(head->serial_number))
+   g_string_append_printf(device_id, "-%s", head->serial_number);
  
  	/* no EDID data, so use fallback */

if (strcmp(device_id->str, "xrandr") == 0)
@@ -230,6 +231,7 @@ colord_notifier_output_destroy(struct wl_listener 
*listener, void *data)
  static void
  colord_output_created(struct cms_colord *cms, struct weston_output *o)
  {
+   struct weston_head *head = >head;
CdDevice *device;
const gchar *tmp;
gchar *device_id;
@@ -251,25 +253,25 @@ colord_output_created(struct cms_colord *cms, struct 
weston_output *o)
g_hash_table_insert (device_props,
 g_strdup(CD_DEVICE_PROPERTY_COLORSPACE),
 
g_strdup(cd_colorspace_to_string(CD_COLORSPACE_RGB)));
-   if (edid_value_valid(o->make)) {
-   tmp = g_hash_table_lookup(cms->pnp_ids, o->make);
+   if (edid_value_valid(head->make)) {
+   tmp = g_hash_table_lookup(cms->pnp_ids, head->make);
if (tmp == NULL)
-   tmp = o->make;
+   tmp = head->make;
g_hash_table_insert (device_props,
 g_strdup(CD_DEVICE_PROPERTY_VENDOR),
 g_strdup(tmp));
}
-   if (edid_value_valid(o->model)) {
+   if (edid_value_valid(head->model)) {
g_hash_table_insert (device_props,
 g_strdup(CD_DEVICE_PROPERTY_MODEL),
-g_strdup(o->model));
+g_strdup(head->model));
}
-   if (edid_value_valid(o->serial_number)) {
+   if (edid_value_valid(head->serial_number)) {
g_hash_table_insert (device_props,
 g_strdup(CD_DEVICE_PROPERTY_SERIAL),
-g_strdup(o->serial_number));
+g_strdup(head->serial_number));
}
-   if (o->connection_internal) {
+   if