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.
 
> 
> > +	ptr = vgem_mmap(exporter, scratch, PROT_WRITE);
> 
> Ditto, we should check that ptr != NULL.

Same.

> 
> > +	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.

> > +		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.

> > +		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.

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

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);
> > +}