Re: [Intel-gfx] [PATCH] drm/i915: Add relocation exceptions for two other platforms

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jun 11, 2021 at 08:09:00AM +0200, Zbigniew Kempczyński wrote:
> On Thu, Jun 10, 2021 at 10:36:12AM -0400, Rodrigo Vivi wrote:
> > On Thu, Jun 10, 2021 at 12:39:55PM +0200, Zbigniew Kempczyński wrote:
> > > We have established previously we stop using relocations starting
> > > from gen12 platforms with Tigerlake as an exception. We keep this
> > > statement but we want to enable relocations conditionally for
> > > Rocketlake and Alderlake under require_force_probe flag set.
> > > 
> > > Keeping relocations under require_force_probe flag is interim solution
> > > until IGTs will be rewritten to use softpin.
> > 
> > hmm... to be really honest I'm not so happy that we are introducing
> > a new criteria to the force_probe.
> > 
> > The criteria was to have a functional driver and not to track uapi.
> > 
> > But on the other hand I do recognize that the current definition
> > of the flag allows that, because we have established that with
> > this behavior, the "driver for new Intel graphics devices that
> > are recognized but not properly supported by this kernel version"
> > (as stated in the Kconfig for the DRM_I915_FORCE_PROBE).
> > 
> > However...
> > 
> > > 
> > > v2: - remove inline from function definition (Jani)
> > >     - fix indentation
> > > 
> > > v3: change to GRAPHICS_VER() (Zbigniew)
> > > 
> > > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > > Cc: Dave Airlie <airlied@redhat.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > Acked-by: Dave Airlie <airlied@redhat.com>
> > > ---
> > >  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 24 +++++++++++++++----
> > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > index a8abc9af5ff4..30c4f0549ea0 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > @@ -491,16 +491,30 @@ eb_unreserve_vma(struct eb_vma *ev)
> > >  	ev->flags &= ~__EXEC_OBJECT_RESERVED;
> > >  }
> > >  
> > > +static bool platform_has_relocs_enabled(const struct i915_execbuffer *eb)
> > > +{
> > > +	/*
> > > +	 * Relocations are disallowed starting from gen12 with Tigerlake
> > > +	 * as an exception. We allow temporarily use relocations for Rocketlake
> > > +	 * and Alderlake when require_force_probe flag is set.
> > > +	 */
> > > +	if (GRAPHICS_VER(eb->i915) < 12 || IS_TIGERLAKE(eb->i915))
> > > +		return true;
> > > +
> > > +	if (INTEL_INFO(eb->i915)->require_force_probe &&
> > > +	    (IS_ROCKETLAKE(eb->i915)
> > 
> > This ship has sailed... RKL is not protected by this flag any longer.
> > Should this be on the TGL side now?
> 
> +Lucas
> 
> I think no, RKL has relocations disabled so we cannot put it to TGL side.
> So if RKL is already released then putting it under require_force_probe 
> flag is wrong and only I can do is to remove it from that condition. 
> There's no option to unblock RKL on IGT CI until we rewrite all the tests.
> We have to rely then on ADL* with require_force_probe flag to check how
> ADL will work with relocations. 

So... I'm confused now. I'm missing the point of this patch then.
I thought the reason was to protect from any user space to attempt to
use the relocation, unless using the force_probe temporarily only for
these platforms.
But if I'm understanding correctly now it is only to silence CI?!
Is that the case?
Is the CI noise so bad?

> 
> > 
> > >  || IS_ALDERLAKE_S(eb->i915) ||
> > > +	     IS_ALDERLAKE_P(eb->i915)))
> > 
> > How to ensure that we will easily catch this when removing the
> > flag?
> > 
> > I mean, should we have a GEM_BUG or drm_err message when these
> > platforms in this list has not the required_force_probe?
> 
> I don't think we need GEM_BUG()/drm_err() - when IGT tests will support
> both - reloc + no-reloc - then condition will be limited to:
> 
>         if (GRAPHICS_VER(eb->i915) < 12 || IS_TIGERLAKE(eb->i915))
>                 return true;
>  
>         return false;
> 
> so require_force_probe condition will be deleted and we won't need it
> anymore (IGTs will be ready).

yes...
but then, when we remove the flag we will forget to come here and remove
this check.

Oh, and I just thought that we might need drm_error when the protection
doesn't exist for the platform, but also a drm_info to the user to tell
this is a temporary accepted behavior, but that will be removed later

The concern is if any other userspace was using the flag and suddently move to a
version without the flag, it would be considered a regression...

> 
> --
> Zbigniew
> 
> > 
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  static int
> > >  eb_validate_vma(struct i915_execbuffer *eb,
> > >  		struct drm_i915_gem_exec_object2 *entry,
> > >  		struct i915_vma *vma)
> > >  {
> > > -	/* Relocations are disallowed for all platforms after TGL-LP.  This
> > > -	 * also covers all platforms with local memory.
> > > -	 */
> > > -	if (entry->relocation_count &&
> > > -	    GRAPHICS_VER(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))
> > > +	if (entry->relocation_count && !platform_has_relocs_enabled(eb))
> > >  		return -EINVAL;
> > >  
> > >  	if (unlikely(entry->flags & eb->invalid_flags))
> > > -- 
> > > 2.26.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx