[Intel-gfx] [PATCH 1/1] drm/i915: Start disabling pread/pwrite ioctl's for future platforms

[Jason Ekstrand]

On Fri, Jan 22, 2021 at 4:40 PM Ashutosh Dixit <ashutosh.dixit at intel.com> wrote:
>
> The guidance for i915 at this time is to start phasing out pread/pwrite
> ioctl's, the rationale being (a) the functionality can be done entirely in
> userspace with a combination of mmap + memcpy, and (b) no existing user
> mode clients actually use the pread/pwrite interface.
>
> In this patch we disable these ioctls for dGfx platforms with the
> expectation that this will be done for all future platforms (both discrete
> and integrated). IGT changes which handle this kernel change have also been
> submitted [1].
>
> [1] https://patchwork.freedesktop.org/series/81384/
>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 1 +
>  drivers/gpu/drm/i915/i915_gem.c          | 6 ++++++
>  drivers/gpu/drm/i915/i915_pci.c          | 3 ++-
>  drivers/gpu/drm/i915/intel_device_info.h | 1 +
>  4 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7efb501e22d2..66edffadf4a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1674,6 +1674,7 @@ tgl_stepping_get(struct drm_i915_private *dev_priv)
>  #define HAS_SECURE_BATCHES(dev_priv) (INTEL_GEN(dev_priv) < 6)
>  #define HAS_WT(dev_priv)       HAS_EDRAM(dev_priv)
>
> +#define HAS_PREAD_PWRITE(dev_priv)     (!INTEL_INFO(dev_priv)->no_pread_pwrite)
>  #define HWS_NEEDS_PHYSICAL(dev_priv)   (INTEL_INFO(dev_priv)->hws_needs_physical)
>
>  #define HAS_LOGICAL_RING_CONTEXTS(dev_priv) \
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9b04dff5eb32..9f3ef68fadf3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -382,6 +382,9 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
>         struct drm_i915_gem_object *obj;
>         int ret;
>
> +       if (!HAS_PREAD_PWRITE(to_i915(dev)))
> +               return -EOPNOTSUPP;
> +
>         if (args->size == 0)
>                 return 0;
>
> @@ -687,6 +690,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>         struct drm_i915_gem_object *obj;
>         int ret;
>
> +       if (!HAS_PREAD_PWRITE(to_i915(dev)))
> +               return -EOPNOTSUPP;
> +
>         if (args->size == 0)
>                 return 0;
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 6cff7cf0f17b..3231d989275c 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -910,7 +910,8 @@ static const struct intel_device_info rkl_info = {
>         .has_master_unit_irq = 1, \
>         .has_llc = 0, \
>         .has_snoop = 1, \
> -       .is_dgfx = 1
> +       .is_dgfx = 1, \
> +       .no_pread_pwrite = 1
>
>  static const struct intel_device_info dg1_info __maybe_unused = {
>         GEN12_DGFX_FEATURES,
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index e6ca1023ffcf..8edd2cfb0bab 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -114,6 +114,7 @@ enum intel_ppgtt_type {
>         func(is_lp); \
>         func(require_force_probe); \
>         func(is_dgfx); \
> +       func(no_pread_pwrite); \

Is it really better to add a device_info flag here or would we be
better off making it dependent on HW generation?  This is what I did
for relocs:

       /* Relocations are disallowed for all platforms after TGL-LP */
       if (INTEL_GEN(eb->i915) >= 12 && !IS_TIGERLAKE(eb->i915))
               return -EINVAL;

       /* All discrete memory platforms are Gen12 or above */
       BUG_ON(HAS_LMEM(eb->i915));

At least to me, that makes the code more obvious.

--Jason