Re: [igt-dev] [PATCH i-g-t v2 1/2] lib/fb: introduce __igt_create_fb_with_bo_size() and __igt_create_fb()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 14, 2018 at 07:07:35PM +0200, Martin Peres wrote:
> They are the same as their non-underscored counterparts, except they
> will return the error rather than dying when an error occurs.
> 
> Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
> ---
>  lib/igt_fb.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>  lib/igt_fb.h |   8 +++++
>  2 files changed, 94 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index ecd73053..a96fbaba 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -751,6 +751,7 @@ void igt_paint_image(cairo_t *cr, const char *filename,
>   * @fb: pointer to an #igt_fb structure
>   * @bo_size: size of the backing bo (0 for automatic size)
>   * @bo_stride: stride of the backing bo (0 for automatic stride)
> + * @fb_id: if not NULL, returns the kms id of the created framebuffer
>   *
>   * This function allocates a gem buffer object suitable to back a framebuffer
>   * with the requested properties and then wraps it up in a drm framebuffer
> @@ -759,18 +760,17 @@ void igt_paint_image(cairo_t *cr, const char *filename,
>   * The backing storage of the framebuffer is filled with all zeros, i.e. black
>   * for rgb pixel formats.
>   *
> - * Returns:
> - * The kms id of the created framebuffer.
> + * Returns: 0 in case of success, the error otherwise.
>   */
> -unsigned int
> -igt_create_fb_with_bo_size(int fd, int width, int height,
> -			   uint32_t format, uint64_t tiling,
> -			   struct igt_fb *fb, unsigned bo_size,
> -			   unsigned bo_stride)
> +int
> +__igt_create_fb_with_bo_size(int fd, int width, int height,
> +			     uint32_t format, uint64_t tiling,
> +			     struct igt_fb *fb, unsigned bo_size,
> +			     unsigned bo_stride, uint32_t *fb_id /* out */)

We'll stuff the fb_id into fb->fb_id, so I think we shouldn't need 
the the out parameter.

Apart from that things look reasonable to me.

>  {
>  	struct format_desc_struct *f = lookup_drm_format(format);
> -	uint32_t fb_id;
> -	int i;
> +	uint32_t _fb_id;
> +	int ret, i;
>  
>  	igt_assert_f(f, "DRM format %08x not found\n", format);
>  
> @@ -789,9 +789,14 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
>  
>  	if (tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
>  	    tiling != LOCAL_I915_FORMAT_MOD_X_TILED) {
> -		do_or_die(__kms_addfb(fd, fb->gem_handle, width, height,
> -				      fb->stride, format, tiling, fb->offsets,
> -				      LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id));
> +		ret = __kms_addfb(fd, fb->gem_handle, width, height,
> +				  fb->stride, format, tiling, fb->offsets,
> +				  LOCAL_DRM_MODE_FB_MODIFIERS, &_fb_id);
> +		if (ret) {
> +			igt_debug("%s: __kms_addfb() failed: %d (%s)\n",
> +				  __func__, errno, strerror(errno));
> +			return ret;
> +		}
>  	} else {
>  		uint32_t handles[4];
>  		uint32_t pitches[4];
> @@ -806,16 +811,21 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
>  			pitches[i] = fb->stride;
>  		}
>  
> -		do_or_die(drmModeAddFB2(fd, width, height, format,
> -					handles, pitches, fb->offsets,
> -					&fb_id, 0));
> +		ret = drmModeAddFB2(fd, width, height, format,
> +		                    handles, pitches, fb->offsets,
> +		                    &_fb_id, 0);
> +		if (ret) {
> +			igt_debug("%s: drmModeAddFB2() failed: %d (%s)\n",
> +				  __func__, errno, strerror(errno));
> +			return ret;
> +		}
>  	}
>  
>  	fb->width = width;
>  	fb->height = height;
>  	fb->tiling = tiling;
>  	fb->drm_format = format;
> -	fb->fb_id = fb_id;
> +	fb->fb_id = _fb_id;
>  	fb->fd = fd;
>  	fb->num_planes = f->planes ?: 1;
>  	fb->plane_bpp[0] = f->bpp;
> @@ -829,6 +839,44 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
>  		fb->plane_width[i] = planar_width(f, width, i);
>  	}
>  
> +	if (fb_id)
> +		*fb_id = _fb_id;
> +
> +	return 0;
> +}
> +
> +/**
> + * igt_create_fb_with_bo_size:
> + * @fd: open i915 drm file descriptor
> + * @width: width of the framebuffer in pixel
> + * @height: height of the framebuffer in pixel
> + * @format: drm fourcc pixel format code
> + * @tiling: tiling layout of the framebuffer (as framebuffer modifier)
> + * @fb: pointer to an #igt_fb structure
> + * @bo_size: size of the backing bo (0 for automatic size)
> + * @bo_stride: stride of the backing bo (0 for automatic stride)
> + *
> + * This function allocates a gem buffer object suitable to back a framebuffer
> + * with the requested properties and then wraps it up in a drm framebuffer
> + * object of the requested size. All metadata is stored in @fb.
> + *
> + * The backing storage of the framebuffer is filled with all zeros, i.e. black
> + * for rgb pixel formats.
> + *
> + * Returns:
> + * The kms id of the created framebuffer.
> + */
> +unsigned int
> +igt_create_fb_with_bo_size(int fd, int width, int height,
> +			   uint32_t format, uint64_t tiling,
> +			   struct igt_fb *fb, unsigned bo_size,
> +			   unsigned bo_stride)
> +{
> +	uint32_t fb_id;
> +
> +	do_or_die(__igt_create_fb_with_bo_size(fd, width, height, format,
> +	                                       tiling, fb, bo_size, bo_stride,
> +	                                       &fb_id));
>  	return fb_id;
>  }
>  
> @@ -858,6 +906,28 @@ unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
>  					  0, 0);
>  }
>  
> +/**
> + * __igt_create_fb:
> + * @fd: open i915 drm file descriptor
> + * @width: width of the framebuffer in pixel
> + * @height: height of the framebuffer in pixel
> + * @format: drm fourcc pixel format code
> + * @tiling: tiling layout of the framebuffer
> + * @fb: pointer to an #igt_fb structure
> + * @fb_id: if not NULL, returns the kms id of the created framebuffer
> + *
> + * Same as igt_create_fb, but return an error if it failed rather than failing.
> + *
> + * Returns: 0 in case of success, the error otherwise.
> + */
> +int __igt_create_fb(int fd, int width, int height, uint32_t format,
> +		    uint64_t tiling, struct igt_fb *fb,
> +		    uint32_t *fb_id /* out */)
> +{
> +	return __igt_create_fb_with_bo_size(fd, width, height, format, tiling,
> +					    fb, 0, 0, fb_id);
> +}
> +
>  /**
>   * igt_create_color_fb:
>   * @fd: open i915 drm file descriptor
> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> index 023b069d..8a8bb34d 100644
> --- a/lib/igt_fb.h
> +++ b/lib/igt_fb.h
> @@ -102,11 +102,19 @@ void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp,
>  			  unsigned *width_ret, unsigned *height_ret);
>  void igt_calc_fb_size(int fd, int width, int height, uint32_t format, uint64_t tiling,
>  		      unsigned *size_ret, unsigned *stride_ret);
> +int
> +__igt_create_fb_with_bo_size(int fd, int width, int height,
> +			     uint32_t format, uint64_t tiling,
> +			     struct igt_fb *fb, unsigned bo_size,
> +			     unsigned bo_stride, uint32_t *fb_id /* out */);
>  unsigned int
>  igt_create_fb_with_bo_size(int fd, int width, int height,
>  			   uint32_t format, uint64_t tiling,
>  			   struct igt_fb *fb, unsigned bo_size,
>  			   unsigned bo_stride);
> +int __igt_create_fb(int fd, int width, int height, uint32_t format,
> +		    uint64_t tiling, struct igt_fb *fb,
> +		    uint32_t *fb_id /* out */);
>  unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
>  			   uint64_t tiling, struct igt_fb *fb);
>  unsigned int igt_create_color_fb(int fd, int width, int height,
> -- 
> 2.16.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev