Re: [PATCH 15/16] drm/i915: Repack fence tiling mode and stride into a single integer

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 15/16] drm/i915: Repack fence tiling mode and stride into a single integer
Date: Thu, 04 Aug 2016 14:36:42 +0300	[thread overview]
Message-ID: <1470310602.3633.52.camel@linux.intel.com> (raw)
In-Reply-To: <20160804113411.GS12611@nuc-i3427.alporthouse.com>

On to, 2016-08-04 at 12:34 +0100, Chris Wilson wrote:
> On Thu, Aug 04, 2016 at 02:17:22PM +0300, Joonas Lahtinen wrote:
> > 
> > > 
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2465,9 +2465,8 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
> > >  		return false;
> > >  	}
> > >  
> > > -	obj->tiling_mode = plane_config->tiling;
> > > -	if (obj->tiling_mode == I915_TILING_X)
> > > -		obj->stride = fb->pitches[0];
> > > +	if (plane_config->tiling == I915_TILING_X)
> > > +		obj->tiling_and_stride = fb->pitches[0] | I915_TILING_X;
> > This is not equivalent code.
> Setting tiling mode and not stride is illegal, as is setting stride for
> I915_TILING_NONE. Initial tiling_and_stride here is 0 (from object
> allocation), so the result is the same.
> 
> What did I miss?
> 

Patch splitting. If you mix a couple hundred line change of mechanical
changes to some functional improvements, that's really depressing to
review.

Regards, Joonas

> > 
> > > 
> > >  	} else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> > > @@ -14919,15 +14918,15 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > >  	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> > >  		/* Enforce that fb modifier and tiling mode match, but only for
> > >  		 * X-tiled. This is needed for FBC. */
> > > -		if (!!(obj->tiling_mode == I915_TILING_X) !=
> > > +		if (!!(i915_gem_object_get_tiling(obj) == I915_TILING_X) !=
> > A note, !! are redundant as == returns bool already. Could remove while
> > touching.
> Petition Daniel, maybe we can now have I915_TILING_Y support here as
> well so the API is not intentionally left incomplete. Grr.
> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx