[i-g-t,v2,05/13] igt: fb: Move size computation to the common path

Submitted by Maxime Ripard on Jan. 8, 2019, 3:19 p.m.

Details

Message ID 20190108152001.5367-5-maxime.ripard@bootlin.com
State New
Series "igt: chamelium: Test YUV buffers using the Chamelium"
Headers show

Commit Message

Maxime Ripard Jan. 8, 2019, 3:19 p.m.
In order to properly support the YUV buffer on non-i915 platforms, we need
to run calc_fb_size for the dumb buffers allocation path too. Move it out
of the special case and in the common code path.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 lib/igt_fb.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index d69c3fb2d38d..31e44d6188d9 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -528,16 +528,14 @@  static void clear_yuv_buffer(struct igt_fb *fb)
 /* helpers to create nice-looking framebuffers */
 static int create_bo_for_fb(struct igt_fb *fb)
 {
+	uint64_t size = calc_fb_size(fb);
 	int fd = fb->fd;
 
+	/* respect the size requested by the caller */
+	if (fb->size == 0)
+		fb->size = size;
+
 	if (fb->tiling || fb->size || fb->strides[0] || igt_format_is_yuv(fb->drm_format)) {
-		uint64_t size;
-
-		size = calc_fb_size(fb);
-
-		/* respect the size requested by the caller */
-		if (fb->size == 0)
-			fb->size = size;
 
 		fb->is_dumb = false;
 

Comments

Paul Kocialkowski Jan. 10, 2019, 10:10 a.m.
On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> In order to properly support the YUV buffer on non-i915 platforms, we need
> to run calc_fb_size for the dumb buffers allocation path too. Move it out
> of the special case and in the common code path.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

> ---
>  lib/igt_fb.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d69c3fb2d38d..31e44d6188d9 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -528,16 +528,14 @@ static void clear_yuv_buffer(struct igt_fb *fb)
>  /* helpers to create nice-looking framebuffers */
>  static int create_bo_for_fb(struct igt_fb *fb)
>  {
> +	uint64_t size = calc_fb_size(fb);
>  	int fd = fb->fd;
>  
> +	/* respect the size requested by the caller */
> +	if (fb->size == 0)
> +		fb->size = size;
> +
>  	if (fb->tiling || fb->size || fb->strides[0] || igt_format_is_yuv(fb->drm_format)) {
> -		uint64_t size;
> -
> -		size = calc_fb_size(fb);
> -
> -		/* respect the size requested by the caller */
> -		if (fb->size == 0)
> -			fb->size = size;
>  
>  		fb->is_dumb = false;
>
Paul Kocialkowski Jan. 10, 2019, 10:43 a.m.
Hi,

On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> In order to properly support the YUV buffer on non-i915 platforms, we need
> to run calc_fb_size for the dumb buffers allocation path too. Move it out
> of the special case and in the common code path.

Actually, I have one minor comment below.

> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  lib/igt_fb.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d69c3fb2d38d..31e44d6188d9 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -528,16 +528,14 @@ static void clear_yuv_buffer(struct igt_fb *fb)
>  /* helpers to create nice-looking framebuffers */
>  static int create_bo_for_fb(struct igt_fb *fb)
>  {
> +	uint64_t size = calc_fb_size(fb);
>  	int fd = fb->fd;
>  
> +	/* respect the size requested by the caller */
> +	if (fb->size == 0)
> +		fb->size = size;
> +
>  	if (fb->tiling || fb->size || fb->strides[0] || igt_format_is_yuv(fb->drm_format)) {

Calling calc_fb_size when the function begins will automatically set
the strides if it was previously 0, so we can drop the "|| fb-
>strides[0]" here (will never happen).

Generally speaking, I think it would be much more simple to always go
with i915 gem create instead of dumb alloc as soon as i915 is detected
and drop all these conditions.

I can't see why dumb buffer would be preferable, but I might be missing
something here (i915 developers, feel free to point it out if that's
the case)!

Cheers,

Paul

> -		uint64_t size;
> -
> -		size = calc_fb_size(fb);
> -
> -		/* respect the size requested by the caller */
> -		if (fb->size == 0)
> -			fb->size = size;
>  
>  		fb->is_dumb = false;
>