Re: [igt-dev] [PATCH i-g-t 2/6] kms_writeback: Add initial writeback tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 26-06-18 om 10:38 schreef Liviu Dudau:
> On Mon, Jun 25, 2018 at 02:48:47PM +0200, Maarten Lankhorst wrote:
>> Op 01-03-18 om 18:38 schreef Liviu Dudau:
>>> From: Brian Starkey <brian.starkey@xxxxxxx>
>>>
>>> Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and
>>> WRITEBACK_FB_ID properties on writeback connectors, ensuring their
>>> behaviour is correct.
>>>
>>> Signed-off-by: Brian Starkey <brian.starkey@xxxxxxx>
>>> ---
>>>  lib/igt_kms.c          |   7 +-
>>>  lib/igt_kms.h          |   8 ++
>>>  tests/Makefile.sources |   1 +
>>>  tests/kms_writeback.c  | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  tests/meson.build      |   1 +
>>>  5 files changed, 365 insertions(+), 4 deletions(-)
>>>  create mode 100644 tests/kms_writeback.c
>>>
>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>> index 00d9d2e2..d558c1b8 100644
>>> --- a/lib/igt_kms.c
>>> +++ b/lib/igt_kms.c
>>> @@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>>>  /*
>>>   * Add position and fb changes of a plane to the atomic property set
>>>   */
>>> -static void
>>> -igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>>  	drmModeAtomicReq *req)
>>>  {
>>>  	igt_display_t *display = pipe->display;
>>> @@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
>>>  /*
>>>   * Add crtc property changes to the atomic property set
>>>   */
>>> -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>>> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>>>  {
>>>  	int i;
>>>  
>>> @@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>>>  /*
>>>   * Add connector property changes to the atomic property set
>>>   */
>>> -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>>> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>>>  {
>>>  
>>>  	int i;
>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>>> index 59ccc4c6..f38fd0a0 100644
>>> --- a/lib/igt_kms.h
>>> +++ b/lib/igt_kms.h
>>> @@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
>>>  					 enum igt_atomic_connector_properties prop,
>>>  					 const void *ptr, size_t length);
>>>  
>>> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> +	drmModeAtomicReq *req);
>>> +
>>> +
>>> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
>>> +
>>>  /**
>>>   * igt_pipe_obj_has_prop:
>>>   * @pipe: Pipe to check.
>>> @@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
>>>  
>>>  void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
>>>  
>>> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
>>> +
>>>  void igt_enable_connectors(void);
>>>  void igt_reset_connectors(void);
>> Please don't..
> More context on what I should not do/say/write would be helpful.
>
>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>> index 23f859be..9cfa474d 100644
>>> --- a/tests/Makefile.sources
>>> +++ b/tests/Makefile.sources
>>> @@ -212,6 +212,7 @@ TESTS_progs = \
>>>  	kms_tv_load_detect \
>>>  	kms_universal_plane \
>>>  	kms_vblank \
>>> +	kms_writeback \
>>>  	meta_test \
>>>  	perf \
>>>  	perf_pmu \
>>> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
>>> new file mode 100644
>>> index 00000000..d922213d
>>> --- /dev/null
>>> +++ b/tests/kms_writeback.c
>>> @@ -0,0 +1,352 @@
>>> +/*
>>> + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the next
>>> + * paragraph) shall be included in all copies or substantial portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>>> + * IN THE SOFTWARE.
>>> + *
>>> + */
>>> +
>>> +#include <errno.h>
>>> +#include <stdbool.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +
>>> +#include "igt.h"
>>> +#include "igt_core.h"
>>> +#include "igt_fb.h"
>>> +
>>> +/* We need to define these ourselves until we get an updated libdrm */
>>> +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
>>> +#define DRM_MODE_CONNECTOR_WRITEBACK   18
>>> +#endif
>>> +
>>> +static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output)
>>> +{
>>> +	drmModePropertyBlobRes *blob = NULL;
>>> +	uint64_t blob_id;
>>> +	int ret;
>>> +
>>> +	ret = kmstest_get_property(output->display->drm_fd,
>>> +				   output->config.connector->connector_id,
>>> +				   DRM_MODE_OBJECT_CONNECTOR,
>>> +				   igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS],
>>> +				   NULL, &blob_id, NULL);
>>> +	if (ret)
>>> +		blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id);
>>> +
>>> +	igt_assert(blob);
>>> +
>>> +	return blob;
>>> +}
>>> +
>>> +static bool check_writeback_config(igt_display_t *display, igt_output_t *output)
>>> +{
>>> +	igt_fb_t input_fb, output_fb;
>>> +	igt_plane_t *plane;
>>> +	uint32_t writeback_format = DRM_FORMAT_XRGB8888;
>>> +	uint64_t tiling = igt_fb_mod_to_tiling(0);
>>> +	int width, height, ret;
>>> +	drmModeModeInfo override_mode = {
>>> +		.clock = 25175,
>>> +		.hdisplay = 640,
>>> +		.hsync_start = 656,
>>> +		.hsync_end = 752,
>>> +		.htotal = 800,
>>> +		.hskew = 0,
>>> +		.vdisplay = 480,
>>> +		.vsync_start = 490,
>>> +		.vsync_end = 492,
>>> +		.vtotal = 525,
>>> +		.vscan = 0,
>>> +		.vrefresh = 60,
>>> +		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>>> +		.name = {"640x480-60"},
>>> +	};
>>> +	igt_output_override_mode(output, &override_mode);
>> Could we perhaps update igt_output_is_connected to always return true when mode override
>> is set, so we don't have to force enable the connector?
> Yes, that is now obsolete as we have a client cap as well that will hide
> the connector unless set, so this patch needs updating as well.
>
>>> +	width = override_mode.hdisplay;
>>> +	height = override_mode.vdisplay;
>>> +
>>> +	ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888,
>>> +			    tiling, &input_fb);
>>> +	igt_assert(ret >= 0);
>>> +
>>> +	ret = igt_create_fb(display->drm_fd, width, height, writeback_format,
>>> +			    tiling, &output_fb);
>>> +	igt_assert(ret >= 0);
>>> +
>>> +	plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>>> +	igt_plane_set_fb(plane, &input_fb);
>>> +	igt_output_set_writeback_fb(output, &output_fb);
>>> +
>>> +	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
>>> +					    DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>>> +	igt_plane_set_fb(plane, NULL);
>>> +	igt_remove_fb(display->drm_fd, &input_fb);
>>> +	igt_remove_fb(display->drm_fd, &output_fb);
>>> +
>>> +	return !ret;
>>> +}
>>> +
>>> +static igt_output_t *kms_writeback_get_output(igt_display_t *display)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < display->n_outputs; i++) {
>>> +		igt_output_t *output = &display->outputs[i];
>>> +		int j;
>>> +
>>> +		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
>>> +			continue;
>>> +
>>> +		kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON);
>>> +
>>> +		for (j = 0; j < igt_display_get_n_pipes(display); j++) {
>>> +			igt_output_set_pipe(output, j);
>>> +
>>> +			if (check_writeback_config(display, output)) {
>>> +				igt_debug("Using connector %u:%s on pipe %d\n",
>>> +					  output->config.connector->connector_id,
>>> +					  output->name, j);
>>> +				return output;
>>> +			}
>>> +		}
>>> +
>>> +		/* Restore any connectors we don't use, so we don't trip on them later */
>>> +		kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED);
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +static void check_writeback_fb_id(igt_output_t *output)
>>> +{
>>> +	bool found;
>>> +	uint64_t check_fb_id;
>>> +
>>> +	found = kmstest_get_property(output->display->drm_fd, output->id,
>>> +				     DRM_MODE_OBJECT_CONNECTOR,
>>> +				     igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
>>> +				     NULL, &check_fb_id, NULL);
>>> +	igt_assert(found && (check_fb_id == 0));
>>> +}
>>> +
>>> +static int do_writeback_test(igt_output_t *output, uint32_t flags,
>>> +			      uint32_t fb_id, int32_t *out_fence_ptr,
>>> +			      bool ptr_valid)
>>> +{
>>> +	int ret;
>>> +	enum pipe pipe;
>>> +	drmModeAtomicReq *req;
>>> +	igt_display_t *display = output->display;
>>> +	struct kmstest_connector_config *config = &output->config;
>>> +
>>> +	req = drmModeAtomicAlloc();
>>> +	drmModeAtomicSetCursor(req, 0);
>>> +
>>> +	for_each_pipe(display, pipe) {
>>> +		igt_pipe_t *pipe_obj = &display->pipes[pipe];
>>> +		igt_plane_t *plane;
>>> +
>>> +		/*
>>> +		 * Add CRTC Properties to the property set
>>> +		 */
>>> +		igt_atomic_prepare_crtc_commit(pipe_obj, req);
>>> +
>>> +		for_each_plane_on_pipe(display, pipe, plane) {
>>> +			igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
>>> +		}
>>> +	}
>>> +
>>> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
>>> +						  output->props[IGT_CONNECTOR_CRTC_ID],
>>> +						  config->crtc->crtc_id));
>> igt_output_set_pipe() ?
>> Or failing that, igt_output_set_property(output, IGT_CONNECTOR_CRTC_ID);
> Thanks for the hint, will update the patch.
>
>>> +	igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
>>> +						  output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
>>> +						  fb_id));
>> igt_output_set_property(output, IGT_CONNECTOR_FB_ID); or the convenience function from v1.. which also saves you from having to do the call below.
>>
>> I would really keep all the logic for the atomic properties internal, please don't override igt_display_commit_atomic.. we've made it too easy to set properties to anything you want.
> Understood. On the other hand, some flexibility in the test framework
> for whacky values in order to do adversarial testing could be useful.
We do allow this explicitly, see kms_atomic, crtc_invalid_params tries setting an invalid mode:

	/* Pass a series of invalid object IDs for the mode ID. */
	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, plane->drm_plane->plane_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, pipe->crtc_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, output->id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

	igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, fb->fb_id);
	crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);

I couldn't put in a pointer to the kitchen sink, else I would have. O:)
Same works for plane and connector too, this is why you don't need to
override atomic commit(). 

You can put together an atomic test without ever using anything but
igt_plane_set_prop_value, igt_output_set_prop_value and igt_pipe(,_obj)_set_prop_value.

The convenience functions are just for allowing legacy support to continue working.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux