[igt-dev] [PATCH i-g-t 17/25] lib/igt_fb: Remove the hand rolled addfb2

[Petri Latvala]

On Fri, Sep 21, 2018 at 04:15:34PM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-07-19 às 18:04 +0300, Ville Syrjala escreveu:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > Just use __kms_addfb() always instead of hand rolling another
> > drmModeAddFB2() usage. __kms_addfb() demands support for modifiers
> > but we've had that for a long time now so it's not a problem.
> 
> Dear IGT mainainers, do we have any specific rules/requirements about
> running new IGT in older Kernels? A minimum/maximum age?


Nothing specific written down. The thumb rule is that tests must
behave well on all kernels, and particularly well on at least
linus/master and a couple of LTS kernels.

What behave means is the gist of the question. If the poked interface
is not available, the test must skip, and otherwise it should produce
a pass/fail based on the observed effects; when the interface is used
like actual userspace would use it.

If I'm reading this right, this patch will change test results from
pass to skip on really really old kernels (how long is "a long time"
here?). In this particular case, Arek is telling me it's somewhere
around 3.19 so hey.

In the general case, considering the main use of IGT tests is
regression testing in CI, stable should already be in a working
condition (YMMV) so close to zero coverage is lost by requiring modern
stuff. As long as the change is towards skip, not towards failing.

tl;dr: Case-by-case basis, keep stuff working if you can, require more
if you must for making improvements in tests.



-- 
Petri Latvala


> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  lib/igt_fb.c | 35 +++++++----------------------------
> >  1 file changed, 7 insertions(+), 28 deletions(-)
> > 
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index 77611b593a7e..648f0a318c91 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -856,6 +856,7 @@ igt_create_fb_with_bo_size(int fd, int width, int
> > height,
> >  	enum igt_color_encoding color_encoding =
> > IGT_COLOR_YCBCR_BT709;
> >  	enum igt_color_range color_range =
> > IGT_COLOR_YCBCR_LIMITED_RANGE;
> >  	struct format_desc_struct *f = lookup_drm_format(format);
> > +	uint32_t pitches[4];
> >  	uint32_t fb_id;
> >  	int i;
> >  
> > @@ -875,35 +876,13 @@ igt_create_fb_with_bo_size(int fd, int width,
> > int height,
> >  	igt_debug("%s(handle=%d, pitch=%d)\n",
> >  		  __func__, fb->gem_handle, fb->stride);
> >  
> > -	if (tiling != LOCAL_DRM_FORMAT_MOD_NONE &&
> > -	    tiling != LOCAL_I915_FORMAT_MOD_X_TILED) {
> > -		uint32_t pitches[4];
> > -
> > -		for (i = 0; i < fb_num_planes(f); i++)
> > -			pitches[i] = fb->stride;
> > -
> > -		do_or_die(__kms_addfb(fd, fb->gem_handle, width,
> > height,
> > -				      format, tiling, pitches, fb-
> > >offsets,
> > -				      fb_num_planes(f),
> > -				      LOCAL_DRM_MODE_FB_MODIFIERS,
> > &fb_id));
> > -	} else {
> > -		uint32_t handles[4];
> > -		uint32_t pitches[4];
> > +	for (i = 0; i < fb_num_planes(f); i++)
> > +		pitches[i] = fb->stride;
> >  
> > -		memset(handles, 0, sizeof(handles));
> > -		memset(pitches, 0, sizeof(pitches));
> > -
> > -		handles[0] = fb->gem_handle;
> > -		pitches[0] = fb->stride;
> > -		for (i = 0; i < fb_num_planes(f); i++) {
> > -			handles[i] = fb->gem_handle;
> > -			pitches[i] = fb->stride;
> > -		}
> > -
> > -		do_or_die(drmModeAddFB2(fd, width, height, format,
> > -					handles, pitches, fb-
> > >offsets,
> > -					&fb_id, 0));
> > -	}
> > +	do_or_die(__kms_addfb(fd, fb->gem_handle, width, height,
> > +			      format, tiling, pitches, fb->offsets,
> 
> So the real difference is that we now pass "tiling" as a modifier as
> opposed to nothing, for the X and linear formats. A quick look at
> intel_framebuffer_init() suggests that this is fine (and required).
> 
> As long as maintainers don't block it:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> But now __kms_addfb() is almost a reimplementation of of
> drmModeAddFB2WithModifiers(), maybe we could use it instead.
> 
> > +			      fb_num_planes(f),
> > +			      LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id));
> >  
> >  	fb->width = width;
> >  	fb->height = height;