[Intel-gfx] [PATCH] drm/i915: disable set/get_tiling ioctl on gen12+

[Souza, Jose]

On Wed, 2019-09-04 at 16:31 +0200, Daniel Vetter wrote:
> On Wed, Sep 4, 2019 at 4:29 PM Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Tue, Sep 3, 2019 at 9:21 PM Souza, Jose <jose.souza at intel.com>
> > wrote:
> > > On Thu, 2019-08-29 at 08:50 +0200, Daniel Vetter wrote:
> > > > On Wed, Aug 28, 2019 at 08:31:27PM +0000, Souza, Jose wrote:
> > > > > On Wed, 2019-08-28 at 21:13 +0100, Chris Wilson wrote:
> > > > > > Quoting Souza, Jose (2019-08-28 21:11:53)
> > > > > > > Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
> > > > > > 
> > > > > > It's using a non-standard for i915 error code, and
> > > > > > get_tiling is
> > > > > > not
> > > > > 
> > > > > Huum should it use ENOTSUPP then?!
> > > > 
> > > > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#recommended-ioctl-return-values
> > > > 
> > > > Per above "feature not supported" -> EOPNOTSUPP.
> > > 
> > > But like Chris said we are not using EOPNOTSUPP in i915,
> > > i915_perf_open_ioctl() and other 2 perf ioctl uses ENOSUPP,
> > > should we
> > > convert those to EOPNOTSUPP?
> > 
> > $ git grep EOPNOTSUP -- drivers/gpu/drm/ | wc -l
> > 114
> > $ git grep ENOTSUP -- drivers/gpu/drm/ | wc -l
> > 32
> 
> Note that most of the ENOTSUP is in drivers, for the drm core it's
> even more clear:
> 
> $ git grep EOPNOTSUP -- drivers/gpu/drm/*c | wc -l
> 83
> $ git grep ENOTSUP -- drivers/gpu/drm/*c | wc -l
> 5
> 
> Cheers, Daniel
> 
> > Plus i915_pmu.c also has a use of EOPNOTSUPP already.
> > 
> > Furthermore the header for EOPNOTSUP has a pretty clear comment:
> > 
> > /* Defined for the NFSv3 protocol */
> > 
> > Above the entore block of error codes containing ENOTSUPP.
> > 
> > So given all that, plus that we've decided to go with EOPNOTSUPP as
> > the drm-wide recommendation: EOPNOTSUPP it is.

Reviewed-by: José Roberto de Souza <jose.souza at intel.com>

> > 
> > If you disagree, I think there's a pretty substantial patch series
> > for
> > you to type and fix the docs and most users plus explain why we
> > should
> > use an nsf-specific error code (which isn't much worse than the
> > abuse/reinterpretation we currently do, but still I think it's a
> > bit
> > more bending of errno code intentions).
> > 
> > Cheers, Daniel
> > 
> > 
> > 
> > > > > > affected, it will always return LINEAR. You cannot set
> > > > > > tiling as
> > > > > 
> > > > > Following this set_tiling() LINEAR should be allowed too.
> > > > > I prefer the current approach of returning error.
> > > > 
> > > > I'm not seeing the value in keeping get_tiling supported.
> > > > Either
> > > > userspace
> > > > still uses the legacy backhannel and dri2, in which case it
> > > > needs to
> > > > be
> > > > fixed no matter what. Or it's using modifiers, in which case
> > > > this is
> > > > dead
> > > > code. Only other user I can think of is takeover for fastboot,
> > > > but if
> > > > you
> > > > get anything else than untiled it's also broken (we don't have
> > > > an
> > > > ioctl to
> > > > read out the modifiers, heck even all the planes, there's no
> > > > getfb2).
> > > > 
> > > > So really not seeing the point in keeping that working.
> > > > -Daniel
> > 
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
>