[igt-dev] [PATCH i-g-t v2 2/2] tests/kms_properties: Add functional test for "max bpc" property

[Rodrigo Vivi]

On Mon, Oct 29, 2018 at 04:23:18AM -0700, Radhakrishna Sripada wrote:
> On Wed, Oct 24, 2018 at 03:45:35PM -0700, Rodrigo Vivi wrote:
> > On Fri, Oct 12, 2018 at 12:18:46AM -0700, Radhakrishna Sripada wrote:
> > > Test the values in the range advertised by the "max bpc" property.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > > ---
> > >  tests/kms_properties.c | 32 +++++++++++++++++++++++++++++++-
> > >  1 file changed, 31 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/kms_properties.c b/tests/kms_properties.c
> > > index f5a86236f5c8..503a5c251e10 100644
> > > --- a/tests/kms_properties.c
> > > +++ b/tests/kms_properties.c
> > > @@ -82,7 +82,37 @@ static bool ignore_property(uint32_t obj_type, uint32_t prop_flags,
> > >  	return false;
> > >  }
> > >  
> > > -static const struct additional_test property_functional_test[] = {};
> > > +static void max_bpc_prop_test(int fd, uint32_t id, uint32_t type, drmModePropertyPtr prop,
> > > +			      uint32_t prop_id, uint64_t prop_value, bool atomic)
> > > +{
> > > +	drmModeAtomicReqPtr req = NULL;
> > > +	int i, ret;
> > > +
> > > +	if (atomic)
> > > +		req = drmModeAtomicAlloc();
> > > +
> > > +	for ( i = prop->values[0]; i < prop->values[1] ; i++) {
> > > +		if (!atomic) {
> > > +			ret = drmModeObjectSetProperty(fd, id, type, prop_id, i);
> > > +
> > > +			igt_assert_eq(ret, 0);
> > > +		} else {
> > > +			ret = drmModeAtomicAddProperty(req, id, prop_id, i);
> > > +			igt_assert(ret >= 0);
> > > +
> > > +			ret = drmModeAtomicCommit(fd, req, DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> > > +			igt_assert_eq(ret, 0);
> > 
> > Should we read it back to see if it sticks?
> 
> Reading back the property to see if the value sticks IMO is validation 
> of the drm_property infrastructure which is being done by the sanity sub tests.

ahh cool...

> > or should we be checking with some debugfs to see all
> > conditions and to see if the max is getting really respected?
> > 
> We could do this additional check for this property. However it would involve 
> obtaining the corresponding CRTC id and parsing the i915_display_info debugs 
> file to get the appropriate bpp for the CRTC and make sure the bpp is less 
> than 8 * prop->values[i] making the test bulky. Do you suggest a simpler way?

probably a place where selftests could be useful for display?

I don't have the confidence that this test case is validating the feature
itself. We set the max, but we are not sure if the max gets respected.

But it is better to have at least this side of the validation.
For this reasons:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

> 
> Thanks,
> Radhakrishna(RK) Sripada
> > ( really questions not a request ;) )
> > 
> > > +		}
> > > +	}
> > > +
> > > +	if (atomic)
> > > +		drmModeAtomicFree(req);
> > > +}
> > > +
> > > +static const struct additional_test property_functional_test[] = {
> > > +									{"max bpc", DRM_MODE_OBJECT_CONNECTOR,
> > > +									 max_bpc_prop_test},
> > > +								 };
> > >  
> > >  static bool has_additional_test_lookup(uint32_t obj_type, const char *name,
> > >  				bool atomic, int *index)
> > > -- 
> > > 2.9.3
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev