[PATCH weston 10/15] libweston: prevent double weston_output_enable()

[Armin Krezović]

On 04.04.2017 12:58, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
Hi,

> Enabling an already enabled output is an error, at least with the
> current implementation.

Ouch, how embarrasing.

> 
> However, disabling and output that has not been enabled is ok.

an output ------------^

> 
> Cope with the first and document the second.
> 
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Reviewed-by: Armin Krezović <krezovic.armin at gmail.com>

> ---
>  libweston/compositor.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index e7293ea..987dd99 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -4709,7 +4709,8 @@ weston_compositor_add_pending_output(struct weston_output *output,
>  
>  /** Constructs a weston_output object that can be used by the compositor.
>   *
> - * \param output The weston_output object that needs to be enabled.
> + * \param output The weston_output object that needs to be enabled. Must not
> + * be enabled already.
>   *
>   * Output coordinates are calculated and each new output is by default
>   * assigned to the right of previous one.
> @@ -4746,6 +4747,12 @@ weston_output_enable(struct weston_output *output)
>  	struct weston_output *iterator;
>  	int x = 0, y = 0;
>  
> +	if (output->enabled) {
> +		weston_log("Error: attempt to enable an enabled output '%s'\n",
> +			   output->name);
> +		return -1;
> +	}
> +
>  	iterator = container_of(c->output_list.prev,
>  				struct weston_output, link);
>  
> @@ -4826,6 +4833,10 @@ weston_output_enable(struct weston_output *output)
>   *
>   * See weston_output_init() for more information on the
>   * state output is returned to.
> + *
> + * If the output has never been enabled yet, this function can still be
> + * called to ensure that the output is actually turned off rather than left
> + * in the state it was discovered in.
>   */

Again, turning off is only related to drm-backend. I'd make it more generic,
i.e., mention that backend is free to make an output disabled in its own way
(turning crtc off is one way). This explanation ain't bad either, which
is why I gave my review already.

>  WL_EXPORT void
>  weston_output_disable(struct weston_output *output)
> 

Thanks, Armin.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 870 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170404/b58420d0/attachment.sig>