Re: Micro optimizations welcome?

[Giulio Camuffo]

2016-09-18 4:03 GMT+03:00 Yong Bakos :
> On Sep 17, 2016, at 5:42 PM, Ursache Vladimir  wrote:
>>
>> Hi,
>> On amd64 struct wl_display from wayland-client.c can shrink in size
>> from 320 bytes to 304 if two fields (last_error and id) are
>> rearranged. Compiles fine.
>
> I believe you mean last_error and protocol_error.
> This will change the ABI (did you try an ABI check?) and we can't do
> that at this time.

No, wl_display is an opaque type so we can change its size and fields
without problems.

>
> yong
>
>
>
>> End result looks like this:
>>
>> struct wl_display {
>>struct wl_proxy proxy;
>>struct wl_connection *connection;
>>
>>/* When display gets an error event from some object, it stores
>> * information about it here, so that client can get this
>> * information afterwards */
>>struct {
>>/* Code of the error. It can be compared to
>> * the interface's errors enumeration. */
>>uint32_t code;
>>/* id of the proxy that caused the error. There's no warranty
>> * that the proxy is still valid. It's up to client how it will
>> * use it */
>>uint32_t id;
>>/* interface (protocol) in which the error occurred */
>>const struct wl_interface *interface;
>>} protocol_error;
>>/* errno of the last wl_display error */
>>int last_error;
>>int fd;
>>struct wl_map objects;
>>struct wl_event_queue display_queue;
>>struct wl_event_queue default_queue;
>>pthread_mutex_t mutex;
>>
>>int reader_count;
>>uint32_t read_serial;
>>pthread_cond_t reader_cond;
>> };
>> ___
>> wayland-devel mailing list
>> wayland-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Micro optimizations welcome?

[Ursache Vladimir]

 I didn't try an ABI check (ever), I just spotted the padding while
browsing the source code out of curiosity.

As to struct protocol_error (from inside struct wl_display) I moved
the "id" field to avoid compiler padding:

from:
uint32_t
pointer
uint32_t

to:
uint32_t
uint32_t
pointer



On Sun, Sep 18, 2016 at 4:03 AM, Yong Bakos  wrote:
> On Sep 17, 2016, at 5:42 PM, Ursache Vladimir  wrote:
>>
>> Hi,
>> On amd64 struct wl_display from wayland-client.c can shrink in size
>> from 320 bytes to 304 if two fields (last_error and id) are
>> rearranged. Compiles fine.
>
> I believe you mean last_error and protocol_error.
> This will change the ABI (did you try an ABI check?) and we can't do
> that at this time.
>
> yong
>
>
>
>> End result looks like this:
>>
>> struct wl_display {
>>struct wl_proxy proxy;
>>struct wl_connection *connection;
>>
>>/* When display gets an error event from some object, it stores
>> * information about it here, so that client can get this
>> * information afterwards */
>>struct {
>>/* Code of the error. It can be compared to
>> * the interface's errors enumeration. */
>>uint32_t code;
>>/* id of the proxy that caused the error. There's no warranty
>> * that the proxy is still valid. It's up to client how it will
>> * use it */
>>uint32_t id;
>>/* interface (protocol) in which the error occurred */
>>const struct wl_interface *interface;
>>} protocol_error;
>>/* errno of the last wl_display error */
>>int last_error;
>>int fd;
>>struct wl_map objects;
>>struct wl_event_queue display_queue;
>>struct wl_event_queue default_queue;
>>pthread_mutex_t mutex;
>>
>>int reader_count;
>>uint32_t read_serial;
>>pthread_cond_t reader_cond;
>> };
>> ___
>> wayland-devel mailing list
>> wayland-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Micro optimizations welcome?

[Yong Bakos]

On Sep 17, 2016, at 5:42 PM, Ursache Vladimir  wrote:
> 
> Hi,
> On amd64 struct wl_display from wayland-client.c can shrink in size
> from 320 bytes to 304 if two fields (last_error and id) are
> rearranged. Compiles fine.

I believe you mean last_error and protocol_error.
This will change the ABI (did you try an ABI check?) and we can't do
that at this time.

yong



> End result looks like this:
> 
> struct wl_display {
>struct wl_proxy proxy;
>struct wl_connection *connection;
> 
>/* When display gets an error event from some object, it stores
> * information about it here, so that client can get this
> * information afterwards */
>struct {
>/* Code of the error. It can be compared to
> * the interface's errors enumeration. */
>uint32_t code;
>/* id of the proxy that caused the error. There's no warranty
> * that the proxy is still valid. It's up to client how it will
> * use it */
>uint32_t id;
>/* interface (protocol) in which the error occurred */
>const struct wl_interface *interface;
>} protocol_error;
>/* errno of the last wl_display error */
>int last_error;
>int fd;
>struct wl_map objects;
>struct wl_event_queue display_queue;
>struct wl_event_queue default_queue;
>pthread_mutex_t mutex;
> 
>int reader_count;
>uint32_t read_serial;
>pthread_cond_t reader_cond;
> };
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

Micro optimizations welcome?

[Ursache Vladimir]

 Hi,
On amd64 struct wl_display from wayland-client.c can shrink in size
from 320 bytes to 304 if two fields (last_error and id) are
rearranged. Compiles fine.

End result looks like this:

struct wl_display {
struct wl_proxy proxy;
struct wl_connection *connection;

/* When display gets an error event from some object, it stores
 * information about it here, so that client can get this
 * information afterwards */
struct {
/* Code of the error. It can be compared to
 * the interface's errors enumeration. */
uint32_t code;
/* id of the proxy that caused the error. There's no warranty
 * that the proxy is still valid. It's up to client how it will
 * use it */
uint32_t id;
/* interface (protocol) in which the error occurred */
const struct wl_interface *interface;
} protocol_error;
/* errno of the last wl_display error */
int last_error;
int fd;
struct wl_map objects;
struct wl_event_queue display_queue;
struct wl_event_queue default_queue;
pthread_mutex_t mutex;

int reader_count;
uint32_t read_serial;
pthread_cond_t reader_cond;
};
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland] util: Document GCC attributes

[Yong Bakos]

From: Yong Bakos 

Add doxygen comment blocks so these annotations are documented in the html
documentation.

Signed-off-by: Yong Bakos 
---
 src/wayland-util.h | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/wayland-util.h b/src/wayland-util.h
index cacc122..f695266 100644
--- a/src/wayland-util.h
+++ b/src/wayland-util.h
@@ -40,21 +40,28 @@
 extern "C" {
 #endif
 
-/* GCC visibility */
+/** Visibility attribute */
 #if defined(__GNUC__) && __GNUC__ >= 4
 #define WL_EXPORT __attribute__ ((visibility("default")))
 #else
 #define WL_EXPORT
 #endif
 
-/* Deprecated attribute */
+/** Deprecated attribute */
 #if defined(__GNUC__) && __GNUC__ >= 4
 #define WL_DEPRECATED __attribute__ ((deprecated))
 #else
 #define WL_DEPRECATED
 #endif
 
-/* Printf annotation */
+/**
+ * Printf-style argument attribute
+ *
+ * \param x Ordinality of the format string argument
+ * \param y Ordinality of the argument to check against the format string
+ *
+ * \sa https://gcc.gnu.org/onlinedocs/gcc-3.2.1/gcc/Function-Attributes.html
+ */
 #if defined(__GNUC__) && __GNUC__ >= 4
 #define WL_PRINTF(x, y) __attribute__((__format__(__printf__, x, y)))
 #else
-- 
2.7.2

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

Re: [PATCH wayland-protocols] idle-inhibit: Lead with a verb in request description

[Yong Bakos]

On Sep 16, 2016, at 8:42 PM, Bryce Harrington  wrote:
> 
> Signed-off-by: Bryce Harrington 

Reviewed-by: Yong Bakos 

yong


> ---
> unstable/idle-inhibit/idle-inhibit-unstable-v1.xml | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml 
> b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> index 70372fc..9c06cdc 100644
> --- a/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> +++ b/unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
> @@ -42,7 +42,7 @@
> 
> 
>   
> - This destroys the inhibit manager.
> + Destroy the inhibit manager.
>   
> 
> 
> @@ -75,7 +75,7 @@
> 
> 
>   
> - This removes the inhibitor effect from the associated wl_surface.
> + Remove the inhibitor effect from the associated wl_surface.
>   
> 
> 
> -- 
> 1.9.1
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

Re: [PATCH wayland-protocols] input-method: Cleanup some grammar

[Yong Bakos]

On Sep 16, 2016, at 9:37 PM, Bryce Harrington  wrote:
> 
> Fix which vs. that, and rephrase a few descriptions to be clearer.
> 
> Signed-off-by: Bryce Harrington 

Reviewed-by: Yong Bakos 

yong


> ---
> unstable/input-method/input-method-unstable-v1.xml | 25 +++---
> 1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/unstable/input-method/input-method-unstable-v1.xml 
> b/unstable/input-method/input-method-unstable-v1.xml
> index e9d93ba..e454a55 100644
> --- a/unstable/input-method/input-method-unstable-v1.xml
> +++ b/unstable/input-method/input-method-unstable-v1.xml
> @@ -39,7 +39,7 @@
>   commit_state request and are used by the input method to indicate
>   the known text input state in events like preedit_string, commit_string,
>   and keysym. The text input can then ignore events from the input method
> -  which are based on an outdated state (for example after a reset).
> +  that are based on an outdated state (for example after a reset).
> 
>   Warning! The protocol described in this file is experimental and
>   backward incompatible changes may be made. Backward compatible changes
> @@ -55,11 +55,11 @@
> 
> 
>   
> - Send the commit string text for insertion to the application.
> + Send the commit string text to the application for insertion.
> 
> - The text to commit could be either just a single character after a key
> - press or the result of some composing (pre-edit). It could be also an
> - empty text when some text should be removed (see
> + The text could be a single character corresponding to an ordinary key
> + press, one or more characters forming the result of a compose action
> + (pre-edit), or no characters such as when text should be removed (see
>   delete_surrounding_text) or when the input cursor should be moved (see
>   cursor_position).
> 
> @@ -86,10 +86,11 @@
> 
> 
>   
> - Set the styling information on composing text. The style is applied for
> - length in bytes from index relative to the beginning of
> - the composing text (as byte offset). Multiple styles can
> - be applied to a composing text.
> + Set the styling information on a section of the composing text
> + offset index bytes from the beginning and ending at length
> + bytes.
> +
> + Multiple styles can be applied to a composing text.
> 
>   This request should be sent before sending a preedit_string request.
>   
> @@ -100,7 +101,7 @@
> 
> 
>   
> - Set the cursor position inside the composing text (as byte offset)
> + Set the cursor position inside the composing text (as a byte offset)
>   relative to the start of the composing text.
> 
>   When index is negative no cursor should be displayed.
> @@ -245,13 +246,13 @@
>   An input method object is responsible for composing text in response to
>   input from hardware or virtual keyboards. There is one input method
>   object per seat. On activate there is a new input method context object
> -  created which allows the input method to communicate with the text 
> input.
> +  created that allows the input method to communicate with the text 
> input.
> 
> 
> 
>   
>   A text input was activated. Creates an input method context object
> - which allows communication with the text input.
> + that allows communication with the text input.
>   
>   
> 
> -- 
> 1.9.1
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

Re: [PATCH] clients/stacking: Silence a compiler warning

[Armin Krezović]

On 15.09.2016 13:14, Eric Engestrom wrote:
> On Sat, Sep 10, 2016 at 10:55:21PM +0200, Armin Krezović wrote:
>> clang doesn't support gnu_print attribute, so just
>> leave it out when clang is used.
>>
>> Signed-off-by: Armin Krezović 
>> ---
>>  clients/stacking.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/clients/stacking.c b/clients/stacking.c
>> index 4285a17..dd3d338 100644
>> --- a/clients/stacking.c
>> +++ b/clients/stacking.c
>> @@ -184,7 +184,11 @@ fullscreen_handler(struct window *window, void *data)
>>  
>>  static void
>>  draw_string(cairo_t *cr,
>> +#ifndef __clang__
>>  const char *fmt, ...) __attribute__((format (gnu_printf, 2, 
>> 3)));
>> +#else
>> +const char *fmt, ...);
>> +#endif
> 
> I would recommend avoiding structure elements (like `)` and `;`) in
> #ifdef blocks, as some editors get confused by this.
> 
> I would also recommend avoiding code duplication, by moving the
> __attribute__ to its own line and only #ifdef'ing that line.
> (As a reminder, function attributes can also be added before the
> function or between the return type and the function name, avoiding the
> need for `;` to also be on its own line after the #ifdef).
> 

Thanks for the feedback.

> But in this case, I think replacing `gnu_printf` with `printf` is
> probably the best solution.
> I was unable to find the difference between the two, though. Does anyone
> know why one would want to use the gnu-prefixed version?
> 

Pekka asked the same. I have no idea though ...

> I also don't know which project this patch is for (you might want to run
> `git config format.subjectprefix "PATCH $(basename "$PWD")"` in the root
> of each of your local clones), but it might already have a __printf()-style
> macro [1][2][3][4] to avoid using #ifdef everywhere.
> 
Hmm, I used to do this in the X11 backend first (create an output, then
register the API), but was running into some crashes.

With the coldplug introduced, there isn't any listener at backend loading
time, so no harm is done.

For this backend we could move output creation after api registration, but
note that same thing applies to other backends (drm and wayland hotplug).

I also want to note that I couldn't move drm output creation after api
registration (which needs to be registered after compositor->backend has
been inited), as something was expecting initial output create function
to be ran (I didn't investigate much at that point).

Thanks for the hint about subjectprefix. I'll see about using wayland-util.h
provided macro.

> Cheers,
>   Eric
> 
> [1] 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler-gcc.h#n119
> [2] https://cgit.freedesktop.org/mesa/mesa/tree/src/util/macros.h#n128
> [3] https://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-util.h#n59
> [4] https://cgit.freedesktop.org/wayland/libinput/tree/src/libinput.h#n36
> 
>>  
>>  static void
>>  draw_string(cairo_t *cr,
>> -- 
>> 2.10.0




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

Re: [PATCH weston 07/14 v3] weston: Port RDP backend to new output handling API

[Armin Krezović]

On 16.09.2016 15:49, Pekka Paalanen wrote:
> On Thu, 18 Aug 2016 18:42:35 +0200
> Armin Krezović  wrote:
> 
>> This is a complete port of the RDP 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 command line
>> 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:
>>
>>  - Rename output_configure() to output_set_size()
>>in plugin API and describe it.
>>  - Manually fetch parsed_options from wet_compositor.
>>  - Call rdp_output_disable() explicitly from
>>rdp_output_destroy().
>>
>> v3:
>>
>>  - Disallow calling rdp_output_set_size more than once.
>>  - Manually assign a hardcoded name to an output as that's
>>now mandatory.
>>  - Use weston_compositor_add_pending_output().
>>  - Bump weston_rdp_backend_config version to 2.
>>
>> Reviewed-by: Quentin Glidic 
>> Signed-off-by: Armin Krezović 
>> ---
>>  compositor/main.c  |  52 +++--
>>  libweston/compositor-rdp.c | 138 
>> +++--
>>  libweston/compositor-rdp.h |  26 -
>>  3 files changed, 167 insertions(+), 49 deletions(-)
>>
> 
> Hi Armin,
> 

Salut.

> this patch looks essentially good, so:
> Reviewed-by: Pekka Paalanen 
> 

Thanks!

> I did make a couple of comments below that would be nice to address
> later.
> 
> I am also slightly concerned that this patch might introduce a hard to
> lose race during start-up, but I haven't verified it. It's about the
> output initialization vs. accepting RDP clients, I see that the
> backend->output is being used in a few functions. Not quite sure if it
> could get used before it gets initialized, even when main.c enables the
> output ASAP. Hmm... but we probably won't service the listening socket
> before all init is done, so maybe it's not an issue.
> 

In my initial testing I didn't notice any issues. Lets not try to fix what
isn't broken yet.

>> diff --git a/compositor/main.c b/compositor/main.c
>> index 12f5e76..7007901 100644
>> --- a/compositor/main.c
>> +++ b/compositor/main.c
> 
> 
>> diff --git a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
>> index ee81300..b34024a 100644
>> --- a/libweston/compositor-rdp.c
>> +++ b/libweston/compositor-rdp.c
>> @@ -25,6 +25,7 @@
>>  
>>  #include "config.h"
>>  
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -371,15 +372,6 @@ rdp_output_repaint(struct weston_output *output_base, 
>> pixman_region32_t *damage)
>>  return 0;
>>  }
>>  
>> -static void
>> -rdp_output_destroy(struct weston_output *output_base)
>> -{
>> -struct rdp_output *output = to_rdp_output(output_base);
>> -
>> -wl_event_source_remove(output->finish_frame_timer);
>> -free(output);
>> -}
>> -
>>  static int
>>  finish_frame_handler(void *data)
>>  {
>> @@ -471,16 +463,15 @@ rdp_switch_mode(struct weston_output *output, struct 
>> weston_mode *target_mode)
>>  }
>>  
>>  static int
>> -rdp_backend_create_output(struct rdp_backend *b, int width, int height)
>> +rdp_output_set_size(struct weston_output *base,
>> +int width, int height)
>>  {
>> -struct rdp_output *output;
>> -struct wl_event_loop *loop;
>> +struct rdp_output *output = to_rdp_output(base);
>>  struct weston_mode *currentMode;
>>  struct weston_mode initMode;
>>  
>> -output = zalloc(sizeof *output);
>> -if (output == NULL)
>> -return -1;
>> +/* We can only be called once. */
>> +assert(!output->base.current_mode);
>>  
>>  wl_list_init(>peers);
>>  wl_list_init(>base.mode_list);
>> @@ -492,48 +483,100 @@ rdp_backend_create_output(struct rdp_backend *b, int 
>> width, int height)
>>  
>>  currentMode = ensure_matching_mode(>base, );
>>  if (!currentMode)
>> -goto out_free_output;
>> +return -1;
>>  
>>  output->base.current_mode = output->base.native_mode = currentMode;
>> -weston_output_init(>base, b->compositor, 0, 0, width, height,
>> -   WL_OUTPUT_TRANSFORM_NORMAL, 1);
>> -
>>  output->base.make = "weston";
>>  output->base.model = "rdp";
>> +
>> +/* XXX: Calculate proper size. */
>> +output->base.mm_width = width;
>> +output->base.mm_height = height;
>> +
>> +output->base.start_repaint_loop = rdp_output_start_repaint_loop;
>> +output->base.repaint = rdp_output_repaint;
>> +output->base.assign_planes = NULL;
>> +output->base.set_backlight = NULL;
>> +output->base.set_dpms = NULL;
>> +output->base.switch_mode = rdp_switch_mode;
> 
> I'm wondering 

Re: [PATCH weston 04/14 v3] weston: Port DRM backend to new output handling API

[Armin Krezović]

On 15.09.2016 13:37, Pekka Paalanen wrote:
> On Wed, 14 Sep 2016 11:50:34 +0200
> Armin Krezović  wrote:
> 
>> On 13.09.2016 12:49, Pekka Paalanen wrote:
>>> On Thu, 18 Aug 2016 18:42:32 +0200
>>> Armin Krezović  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ć 
 ---
  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 ||