[igt-dev] [PATCH 2/2] tests/prime_generic: add vendor-agnostic prime tests

[Ser, Simon]

On Fri, 2019-08-16 at 15:21 +0100, Vasilev, Oleg wrote:
> On Fri, 2019-08-16 at 11:07 +0100, Ser, Simon wrote:
> > Hi,
> > 
> > Overall this looks good. Here are a few comments.
> > 
> > I agree with Chris and Daniel regarding naming.
> > 
> > On Fri, 2019-07-12 at 17:16 +0300, Oleg Vasilev wrote:
> > > +static bool has_prime_import(int fd)
> > > +{
> > > +	uint64_t value;
> > > +
> > > +	if (drmGetCap(fd, DRM_CAP_PRIME, &value))
> > > +		return false;
> > > +
> > > +	return value & DRM_PRIME_CAP_IMPORT;
> > 
> > This returns a non-zero value when import is supported, but doesn't
> > return true. This can lead to strange bugs.
> > 
> > It's common to use !! to convert to a bool.
> 
> Hi,
> 
> As discussed, this should be still a valid stdbool.
> 
> > > +	vgem_create(exporter, scratch);
> > 
> > The return value should be checked.
> 
> Actually, it is already checked inside the function. AFAIU, in IGT
> helpers usually call their __underscored siblings and assert return
> value.

Looking at the source code, nope:

    if (drmIoctl(fd, DRM_IOCTL_MODE_CREATE_DUMB, &arg))
        return -errno;

> > > +	ptr = vgem_mmap(exporter, scratch, PROT_WRITE);
> > 
> > Ditto, we should check that ptr != NULL.
> 
> Same.

Ditto

> > > +	for (i = 0; i < scratch->size / sizeof(*ptr); ++i)
> > > +		ptr[i] = color;
> > > +
> > > +	munmap(ptr, scratch->size);
> > > +}
> > > +
> > > +static void prepare_fb(int importer, struct vgem_bo *scratch,
> > > struct igt_fb *fb)
> > > +{
> > > +	enum igt_color_encoding color_encoding = IGT_COLOR_YCBCR_BT709;
> > > +	enum igt_color_range color_range =
> > > IGT_COLOR_YCBCR_LIMITED_RANGE;
> > > +
> > > +	igt_init_fb(fb, importer, scratch->width, scratch->height,
> > > +		    DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > > +		    color_encoding, color_range);
> > 
> > Maybe we could populate fb->strides[0] here, so that we don't need to
> > pass it around in import_fb.
> > 
> > > +}
> > > 
> > > +		dmabuf_fd = prime_handle_to_fd(exporter,
> > > scratch.handle);
> > 
> > We should check dmabuf_fd >= 0.
> 
> Same, it is asserted internally.

This one is checked, indeed.

> > > +		gem_close(exporter, scratch.handle);
> > > +
> > > +		prepare_fb(importer, &scratch, &fb);
> > > +		import_fb(importer, &fb, dmabuf_fd, scratch.pitch);
> > 
> > We should release this FB after we're done.
> 
> You mean remove fb? It is done in collect_crc, which I renamed to
> collect_crc_for_fb to make it more clear.

Ah, right.

> > > +		close(dmabuf_fd);
> > > +
> > > +		colors[i].prime_crc.name = "prime";
> > > +		collect_crc(importer, &fb, &display, output,
> > > +			    pipe_crc, colors[i].color,
> > > &colors[i].prime_crc);
> > > +
> > > +		igt_create_color_fb(importer,
> > > +				    mode->hdisplay, mode->vdisplay,
> > > +				    DRM_FORMAT_XRGB8888,
> > > LOCAL_DRM_FORMAT_MOD_NONE,
> > > +				    colors[i].r, colors[i].g,
> > > colors[i].b,
> > > +				    &fb);
> > 
> > Ditto, we should release this FB after we're done.
> 
> Same
> 
> > > +
> > > +		colors[i].direct_crc.name = "direct";
> > > +		collect_crc(importer, &fb, &display, output,
> > > +			    pipe_crc, colors[i].color,
> > > &colors[i].direct_crc);
> > > +	}
> > > +	igt_pipe_crc_free(pipe_crc);
> > > +
> > > +	igt_debug("CRC table:\n");
> > > +	igt_debug("Color\t\tPrime\t\tDirect\n");
> > > +	for (i = 0; i < ARRAY_SIZE(colors); i++) {
> > > +		igt_debug("%#08x\t%.8s\t%.8s\n", colors[i].color,
> > > +			  colors[i].prime_crc.str,
> > > colors[i].direct_crc.str);
> > > +		free(colors[i].prime_crc.str);
> > > +		free(colors[i].direct_crc.str);
> > > +	}
> ...
> > > +
> > > +igt_main
> > > +{
> > > +	igt_fixture {
> > > +		kmstest_set_vt_graphics_mode();
> > 
> > Is this really needed?
> 
> Yeah, it is necessary for every KMS test.

Oh, right, the function documentation is helpful.

> The rest is fixed, thanks for the review. New version is sent.

Thanks!

> Oleg
> 
> > > +	}
> > > +	igt_subtest("crc")
> > 
> > Please add a subtest description. See:
> > https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-Core.html#igt-describe
> > 
> > > +		run_test_crc(DRIVER_VGEM, DRIVER_ANY);
> > > +}