Re: [igt-dev] [PATCH i-g-t v4 05/14] igt: fb: Move size computation to the common path

Paul Kocialkowski
2019-01-22T15:46:34Z

Hi,

On Tue, 2019-01-22 at 15:28 +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>
> ---
> 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..a1368ec4703a 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;
>
> - if (fb->tiling || fb->size || fb->strides[0] || igt_format_is_yuv(fb->drm_format)) {
> - uint64_t size;
> + /* respect the size requested by the caller */
> + if (fb->size == 0)
> + fb->size = size;
>
> - size = calc_fb_size(fb);
> -
> - /* respect the size requested by the caller */
> - if (fb->size == 0)
> - fb->size = size;
> + if (fb->tiling || fb->size || igt_format_is_yuv(fb->drm_format)) {

So in this case, fb->size would also never be zero (it's always set a
few lines before). Looking back at all this, we lose the ability to set
an arbitrary stride and size (unless it's Intel and tiled or YUV),
which is a bit problematic.

I think those things are definitely good to allow and keep, so I guess
we should hit the driver-specific GEM allocation when they occur (as
was the case until now).

In cases where we don't have any driver-specific implementation that
might allow a specific size/stride, I think it would be best to produce
an error rather than falling back on dumb alloc that won't honor what
was requested.

So that implies keeping stride = 0 and size = 0 intact when going
through this function.

I should probably have thought this up on the previous revision before
advising to remove the check on the stride though, sorry.

What do you think?

Cheers,

Paul

> fb->is_dumb = false;
>
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering