On Wed, 14 Sep 2016 11:50:34 +0200
Armin Krezović <krezovic.ar...@gmail.com> wrote:

> On 13.09.2016 12:49, Pekka Paalanen wrote:
> > On Thu, 18 Aug 2016 18:42:32 +0200
> > Armin Krezović <krezovic.ar...@gmail.com> wrote:
> >   
> >> This is a complete port of the DRM backend that uses
> >> recently added output handling API for output
> >> configuration.
> >>
> >> Output can be configured at runtime by passing the
> >> necessary configuration parameters, which can be
> >> filled in manually or obtained from the configuration
> >> file using previously added functionality. It is
> >> required that the scale and transform values are set
> >> using the previously added functionality.
> >>
> >> After everything has been set, output needs to be
> >> enabled manually using weston_output_enable().
> >>
> >> v2:
> >>
> >>  - Added missing drmModeFreeCrtc() to drm_output_enable()
> >>    cleanup list in case of failure.
> >>  - Split drm_backend_disable() into drm_backend_deinit()
> >>    to accomodate for changes in the first patch in the
> >>    series. Moved restoring original crtc to
> >>    drm_output_destroy().
> >>
> >> v3:
> >>
> >>  - Moved origcrtc allocation to drm_output_set_mode().
> >>  - Swapped connector_get_current_mode() and
> >>    drm_output_add_mode() calls in drm_output_set_mode()
> >>    to match current weston.
> >>  - Moved crtc_allocator and connector_allocator update
> >>    from drm_output_enable() to create_output_for_connector()
> >>    to avoid problems when more than one monitor is connected
> >>    at startup and crtc allocator wasn't updated before
> >>    create_output_for_connector() was called second time,
> >>    resulting in one screen being turned off.
> >>  - Moved crtc_allocator and connector_allocator update from
> >>    drm_output_deinit() to drm_output_destroy(), as it
> >>    should not be called on drm_output_disable().
> >>  - Use weston_compositor_add_pending_output().
> >>  - Bump weston_drm_backend_config version to 2.
> >>
> >> Signed-off-by: Armin Krezović <krezovic.ar...@gmail.com>
> >> ---
> >>  compositor/main.c          | 104 ++++++-----
> >>  libweston/compositor-drm.c | 434 
> >> ++++++++++++++++++++++++++-------------------
> >>  libweston/compositor-drm.h |  50 +++---
> >>  3 files changed, 343 insertions(+), 245 deletions(-)  
> > 
> > Hi Armin,
> > 
> > all in all, this patch looks very good. There are a few minor bugs to
> > be squashed, and I did list some future development ideas you are free
> > to ignore.
> > 
> > Please wait for reviews for the whole series before re-sending.
> > 
> > Details inlined below.


> >> +static int
> >> +drm_output_enable(struct weston_output *base)
> >> +{
> >> +  struct drm_output *output = to_drm_output(base);
> >> +  struct drm_backend *b = to_drm_backend(base->compositor);
> >> +  struct weston_mode *m;
> >> +
> >> +  output->dpms_prop = drm_get_prop(b->drm.fd, output->connector, "DPMS");
> >>  
> >>    if (b->use_pixman) {
> >>            if (drm_output_init_pixman(output, b) < 0) {
> >>                    weston_log("Failed to init output pixman state\n");
> >> -                  goto err_output;
> >> +                  goto err_free;
> >>            }
> >>    } else if (drm_output_init_egl(output, b) < 0) {
> >>            weston_log("Failed to init output gl state\n");
> >> -          goto err_output;
> >> +          goto err_free;
> >>    }
> >>  
> >> -  output->backlight = backlight_init(drm_device,
> >> -                                     connector->connector_type);
> >>    if (output->backlight) {  
> > 
> > Any reason you left the backlight_init() call in
> > create_output_for_connector()? I think it would be more logical here,
> > being called only when enabled. It shouldn't matter in practise though.
> >   
> 
> It would, yes. But backlight_init uses struct udev_device, which is passed
> to create_output_for_connector, and not preserved anywhere.

Hi,

Ooh, ok, that is reason enough.

> Sure, I could preserve that one too, but I didn't like the idea (it's enough
> we have to preserve the connector).
> 
> If you still want it to be initialized here, I can preserve struct udev_device
> just fine.
> 
> >>            weston_log("Initialized backlight, device %s\n",
> >>                       output->backlight->path);
> >> @@ -2442,15 +2385,8 @@ create_output_for_connector(struct drm_backend *b,
> >>            weston_log("Failed to initialize backlight\n");
> >>    }


> >> @@ -2578,7 +2644,6 @@ create_outputs(struct drm_backend *b, uint32_t 
> >> option_connector,
> >>    drmModeConnector *connector;
> >>    drmModeRes *resources;
> >>    int i;
> >> -  int x = 0, y = 0;
> >>  
> >>    resources = drmModeGetResources(b->drm.fd);
> >>    if (!resources) {
> >> @@ -2610,21 +2675,15 @@ create_outputs(struct drm_backend *b, uint32_t 
> >> option_connector,
> >>                (option_connector == 0 ||
> >>                 connector->connector_id == option_connector)) {
> >>                    if (create_output_for_connector(b, resources,
> >> -                                                  connector, x, y,
> >> -                                                  drm_device) < 0) {
> >> +                                                  connector, drm_device) 
> >> < 0) {
> >>                            drmModeFreeConnector(connector);  
> 
>                              ---^
> 
> >>                            continue;
> >>                    }
> >> -
> >> -                  x += container_of(b->compositor->output_list.prev,
> >> -                                    struct weston_output,
> >> -                                    link)->width;
> >>            }
> >> -
> >> -          drmModeFreeConnector(connector);  
> > 
> > The Free should be moved into an else-branch instead of removed,
> > shouldn't it?
> > 
> >   
> 
> There's already one above (when if fails). This one would run after
> create_output_for_connector has been ran (since previously the
> connector wasn't preserved).

I meant for the case when connector->connection == DRM_MODE_CONNECTED
is not true.


> >>    }
> >>  
> >> -  if (wl_list_empty(&b->compositor->output_list))
> >> +  if (wl_list_empty(&b->compositor->output_list) &&
> >> +      wl_list_empty(&b->compositor->pending_output_list))
> >>            weston_log("No currently active connector found.\n");
> >>  
> >>    drmModeFreeResources(resources);
> >> @@ -2638,7 +2697,6 @@ update_outputs(struct drm_backend *b, struct 
> >> udev_device *drm_device)
> >>    drmModeConnector *connector;
> >>    drmModeRes *resources;
> >>    struct drm_output *output, *next;
> >> -  int x = 0, y = 0;
> >>    uint32_t connected = 0, disconnects = 0;
> >>    int i;
> >>  
> >> @@ -2664,23 +2722,11 @@ update_outputs(struct drm_backend *b, struct 
> >> udev_device *drm_device)
> >>            connected |= (1 << connector_id);
> >>  
> >>            if (!(b->connector_allocator & (1 << connector_id))) {
> >> -                  struct weston_output *last =
> >> -                          container_of(b->compositor->output_list.prev,
> >> -                                       struct weston_output, link);
> >> -
> >> -                  /* XXX: not yet needed, we die with 0 outputs */
> >> -                  if (!wl_list_empty(&b->compositor->output_list))
> >> -                          x = last->x + last->width;
> >> -                  else
> >> -                          x = 0;
> >> -                  y = 0;
> >>                    create_output_for_connector(b, resources,
> >> -                                              connector, x, y,
> >> -                                              drm_device);
> >> +                                              connector, drm_device);
> >>                    weston_log("connector %d connected\n", connector_id);
> >>  
> >>            }
> >> -          drmModeFreeConnector(connector);  
> > 
> > drmModeFreeConnector() should stay but in a new else-branch, right?

This is a similar case, I'm referring to the else branch of
condition !(b->connector_allocator & (1 << connector_id)).


> > 
> >   
> >>    }
> >>    drmModeFreeResources(resources);
> >>  
> >> @@ -2695,6 +2741,16 @@ update_outputs(struct drm_backend *b, struct 
> >> udev_device *drm_device)
> >>                            drm_output_destroy(&output->base);
> >>                    }
> >>            }
> >> +
> >> +          wl_list_for_each_safe(output, next, 
> >> &b->compositor->pending_output_list,
> >> +                                base.link) {
> >> +                  if (disconnects & (1 << output->connector_id)) {
> >> +                          disconnects &= ~(1 << output->connector_id);
> >> +                          weston_log("connector %d disconnected\n",
> >> +                                 output->connector_id);
> >> +                          drm_output_destroy(&output->base);
> >> +                  }
> >> +          }
> >>    }
> >>  }
> >>  


Thanks,
pq

Attachment: pgpHsMhNJDpCy.pgp
Description: OpenPGP digital signature

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

Reply via email to