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@xxxxxxxxx>
> > > Cc: Dave Airlie <airlied@xxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > Cc: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> > > Acked-by: Dave Airlie <airlied@xxxxxxxxxx>
> > > ---
> > > .../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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
