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

["Ser, Simon" <simon.ser@intel.com>]

On Fri, 2019-08-16 at 14:33 +0000, Ser, Simon wrote:
> 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;

Bleh, I read too fast, and mixed everything up. You are right.

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

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.
> 
> 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);
> > > > +}
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev