[PATCH weston 15/21] compositor-fbdev: simplify FB destroy/unmap/disable

[Ray, Ian (GE Healthcare)]

On 22/09/2017, 17.33, "wayland-devel on behalf of Pekka Paalanen" <wayland-devel-bounces at lists.freedesktop.org on behalf of ppaalanen at gmail.com> wrote:
> 
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> Rename fbdev_frame_buffer_destroy() to fbdev_frame_buffer_unmap()
> because that is what it does. Adding the destruction of hw_surface in it
> makes it the perfect counterpart to fbdev_frame_buffer_map() which
> simplifies the code.
> 
> fbdev_frame_buffer_map() can no longer call that, so just open-code the
> munmap() there. It is an error path, we don't really care about
> failures in an error path.
> 
> The error path of fbdev_output_enable() is converted to call
> buffer_unmap() since that is exactly what it did.
> 
> fbdev_output_disable() became redundant, being identical to
> fbdev_frame_buffer_unmap().
> 
> Invariant: output->hw_surface cannot be non-NULL without output->fb
> being non-NULL. hw_surface wraps the mmapped memory so cannot exist
> without the mmap.
> 
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
>  libweston/compositor-fbdev.c | 52 ++++++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/libweston/compositor-fbdev.c b/libweston/compositor-fbdev.c
> index 402648d0..22100f98 100644
> --- a/libweston/compositor-fbdev.c
> +++ b/libweston/compositor-fbdev.c
> @@ -40,6 +40,7 @@
>  #include <unistd.h>
>  #include <linux/fb.h>
>  #include <linux/input.h>
> +#include <assert.h>
>  
>  #include <libudev.h>
>  
> @@ -335,8 +336,6 @@ fbdev_set_screen_info(int fd, struct fbdev_screeninfo *info)
>  	return 1;
>  }
>  
> -static void fbdev_frame_buffer_destroy(struct fbdev_output *output);
> -
>  /* Returns an FD for the frame buffer device. */
>  static int
>  fbdev_frame_buffer_open(const char *fb_dev,
> @@ -401,8 +400,10 @@ fbdev_frame_buffer_map(struct fbdev_output *output, int fd)
>  	retval = 0;
>  
>  out_unmap:
> -	if (retval != 0 && output->fb != NULL)
> -		fbdev_frame_buffer_destroy(output);
> +	if (retval != 0 && output->fb != NULL) {
> +		munmap(output->fb, output->fb_info.buffer_length);
> +		output->fb = NULL;
> +	}
>  
>  out_close:
>  	if (fd >= 0)
> @@ -412,9 +413,18 @@ out_close:
>  }
>  
>  static void
> -fbdev_frame_buffer_destroy(struct fbdev_output *output)
> +fbdev_frame_buffer_unmap(struct fbdev_output *output)
>  {
> -	weston_log("Destroying fbdev frame buffer.\n");
> +	if (!output->fb) {
> +		assert(!output->hw_surface);
> +		return;
> +	}
> +
> +	weston_log("Unmapping fbdev frame buffer.\n");
> +
> +	if (output->hw_surface)
> +		pixman_image_unref(output->hw_surface);
> +	output->hw_surface = NULL;

nitpick: hw_surface need only be set NULL if it was non-NULL (which
seems to be the pattern elsewhere).  But, in any case:

Reviewed-by: Ian Ray <ian.ray at ge.com>

<snip>