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

[Pekka Paalanen]

On Tue, 26 Sep 2017 06:24:56 +0000
"Ray, Ian (GE Healthcare)" <ian.ray at ge.com> wrote:

> 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:

Hi Ian,

yes, that would be correct as well. I'm just emphasizing that
hw_surface is NULL from here on.

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

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171004/2219a5ac/attachment.sig>