drm/i915: disable set/get_tiling ioctl on gen12+
diff mbox series

Message ID 20190820195451.15671-1-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • drm/i915: disable set/get_tiling ioctl on gen12+
Related show

Commit Message

Daniel Vetter Aug. 20, 2019, 7:54 p.m. UTC
The cpu (de)tiler hw is gone, this stopped being useful. Plus it never
supported any of the fancy new tiling formats, which means userspace
also stopped using the magic side-channel this provides.

This would totally break a lot of the igts, but they're already broken
for the same reasons as userspace on gen12 would be.

v2: Look at ggtt->num_fences instead, that also avoids the need for a
comment (Chris). This also means that gen12 support really needs to
make sure num_fences is set to 0. There is a patch for that, but it
checks for HAS_MAPPABLE_APERTURE, which I'm not sure is the right
thing really. Adding relevant people.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Kenneth Graunke <kenneth@whitecape.org>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Daniele Ceraolo Spurio Aug. 20, 2019, 8:57 p.m. UTC | #1
On 8/20/19 12:54 PM, Daniel Vetter wrote:
> The cpu (de)tiler hw is gone, this stopped being useful. Plus it never
> supported any of the fancy new tiling formats, which means userspace
> also stopped using the magic side-channel this provides.
> 
> This would totally break a lot of the igts, but they're already broken
> for the same reasons as userspace on gen12 would be.
> 
> v2: Look at ggtt->num_fences instead, that also avoids the need for a
> comment (Chris). This also means that gen12 support really needs to
> make sure num_fences is set to 0. There is a patch for that, but it
> checks for HAS_MAPPABLE_APERTURE, which I'm not sure is the right
> thing really. Adding relevant people.
> 

We'd obviously need to make that setting for all gen12+, because TGL 
does have mappable aperture.

Apart from the tiling ioctl, the only place I see where we set tiling is 
intel_alloc_initial_plane_obj(), can the users of that object handle the 
lack of fences gracefully? When I wrote the num_fences=0 patch I was 
expecting display to be unavailable, so I didn't really look at that 
part of the code.

It'd also be nice to be more explicit with fencing since we seem to 
often call i915_vma_pin_iomap, which implicitly applies a fence if 
needed, on objects that can't be tiled or have had a fence assigned a 
few lines before. This is more a nice to have tough, possibly together 
with a split of the "mappable" and "fenceable" attributes of the vma.

Daniele

> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> index ca0c2f451742..e5d1ae8d4dba 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> @@ -313,10 +313,14 @@ int
>   i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
>   			  struct drm_file *file)
>   {
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>   	struct drm_i915_gem_set_tiling *args = data;
>   	struct drm_i915_gem_object *obj;
>   	int err;
>   
> +	if (!dev_priv->ggtt.num_fences)
> +		return -EOPNOTSUPP;
> +
>   	obj = i915_gem_object_lookup(file, args->handle);
>   	if (!obj)
>   		return -ENOENT;
> @@ -402,6 +406,9 @@ i915_gem_get_tiling_ioctl(struct drm_device *dev, void *data,
>   	struct drm_i915_gem_object *obj;
>   	int err = -ENOENT;
>   
> +	if (!dev_priv->ggtt.num_fences)
> +		return -EOPNOTSUPP;
> +
>   	rcu_read_lock();
>   	obj = i915_gem_object_lookup_rcu(file, args->handle);
>   	if (obj) {
>
Ville Syrjälä Aug. 21, 2019, 1:55 p.m. UTC | #2
On Tue, Aug 20, 2019 at 01:57:44PM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 8/20/19 12:54 PM, Daniel Vetter wrote:
> > The cpu (de)tiler hw is gone, this stopped being useful. Plus it never
> > supported any of the fancy new tiling formats, which means userspace
> > also stopped using the magic side-channel this provides.
> > 
> > This would totally break a lot of the igts, but they're already broken
> > for the same reasons as userspace on gen12 would be.
> > 
> > v2: Look at ggtt->num_fences instead, that also avoids the need for a
> > comment (Chris). This also means that gen12 support really needs to
> > make sure num_fences is set to 0. There is a patch for that, but it
> > checks for HAS_MAPPABLE_APERTURE, which I'm not sure is the right
> > thing really. Adding relevant people.
> > 
> 
> We'd obviously need to make that setting for all gen12+, because TGL 
> does have mappable aperture.
> 
> Apart from the tiling ioctl, the only place I see where we set tiling is 
> intel_alloc_initial_plane_obj(), can the users of that object handle the 
> lack of fences gracefully?

Gen4+ display engine has its own tiling controls and doesn't care about
fences. So we should be able to leave the obj tiling set to NONE.

Hmm. Actually I think we should reject tiled framebuffers in the BIOS
fb takeover because fbdev needs a linear view for the memory. No can
do without the fence.

> When I wrote the num_fences=0 patch I was 
> expecting display to be unavailable, so I didn't really look at that 
> part of the code.
> 
> It'd also be nice to be more explicit with fencing since we seem to 
> often call i915_vma_pin_iomap, which implicitly applies a fence if 
> needed, on objects that can't be tiled or have had a fence assigned a 
> few lines before. This is more a nice to have tough, possibly together 
> with a split of the "mappable" and "fenceable" attributes of the vma.
> 
> Daniele
> 
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > index ca0c2f451742..e5d1ae8d4dba 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > @@ -313,10 +313,14 @@ int
> >   i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
> >   			  struct drm_file *file)
> >   {
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >   	struct drm_i915_gem_set_tiling *args = data;
> >   	struct drm_i915_gem_object *obj;
> >   	int err;
> >   
> > +	if (!dev_priv->ggtt.num_fences)
> > +		return -EOPNOTSUPP;
> > +
> >   	obj = i915_gem_object_lookup(file, args->handle);
> >   	if (!obj)
> >   		return -ENOENT;
> > @@ -402,6 +406,9 @@ i915_gem_get_tiling_ioctl(struct drm_device *dev, void *data,
> >   	struct drm_i915_gem_object *obj;
> >   	int err = -ENOENT;
> >   
> > +	if (!dev_priv->ggtt.num_fences)
> > +		return -EOPNOTSUPP;
> > +
> >   	rcu_read_lock();
> >   	obj = i915_gem_object_lookup_rcu(file, args->handle);
> >   	if (obj) {
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Aug. 21, 2019, 3:20 p.m. UTC | #3
On Wed, Aug 21, 2019 at 3:55 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Aug 20, 2019 at 01:57:44PM -0700, Daniele Ceraolo Spurio wrote:
> >
> >
> > On 8/20/19 12:54 PM, Daniel Vetter wrote:
> > > The cpu (de)tiler hw is gone, this stopped being useful. Plus it never
> > > supported any of the fancy new tiling formats, which means userspace
> > > also stopped using the magic side-channel this provides.
> > >
> > > This would totally break a lot of the igts, but they're already broken
> > > for the same reasons as userspace on gen12 would be.
> > >
> > > v2: Look at ggtt->num_fences instead, that also avoids the need for a
> > > comment (Chris). This also means that gen12 support really needs to
> > > make sure num_fences is set to 0. There is a patch for that, but it
> > > checks for HAS_MAPPABLE_APERTURE, which I'm not sure is the right
> > > thing really. Adding relevant people.
> > >
> >
> > We'd obviously need to make that setting for all gen12+, because TGL
> > does have mappable aperture.
> >
> > Apart from the tiling ioctl, the only place I see where we set tiling is
> > intel_alloc_initial_plane_obj(), can the users of that object handle the
> > lack of fences gracefully?
>
> Gen4+ display engine has its own tiling controls and doesn't care about
> fences. So we should be able to leave the obj tiling set to NONE.
>
> Hmm. Actually I think we should reject tiled framebuffers in the BIOS
> fb takeover because fbdev needs a linear view for the memory. No can
> do without the fence.

Yeah I think this is just more fallout from "no more fences in the hw".
-Daniel

>
> > When I wrote the num_fences=0 patch I was
> > expecting display to be unavailable, so I didn't really look at that
> > part of the code.
> >
> > It'd also be nice to be more explicit with fencing since we seem to
> > often call i915_vma_pin_iomap, which implicitly applies a fence if
> > needed, on objects that can't be tiled or have had a fence assigned a
> > few lines before. This is more a nice to have tough, possibly together
> > with a split of the "mappable" and "fenceable" attributes of the vma.
> >
> > Daniele
> >
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > index ca0c2f451742..e5d1ae8d4dba 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > @@ -313,10 +313,14 @@ int
> > >   i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
> > >                       struct drm_file *file)
> > >   {
> > > +   struct drm_i915_private *dev_priv = to_i915(dev);
> > >     struct drm_i915_gem_set_tiling *args = data;
> > >     struct drm_i915_gem_object *obj;
> > >     int err;
> > >
> > > +   if (!dev_priv->ggtt.num_fences)
> > > +           return -EOPNOTSUPP;
> > > +
> > >     obj = i915_gem_object_lookup(file, args->handle);
> > >     if (!obj)
> > >             return -ENOENT;
> > > @@ -402,6 +406,9 @@ i915_gem_get_tiling_ioctl(struct drm_device *dev, void *data,
> > >     struct drm_i915_gem_object *obj;
> > >     int err = -ENOENT;
> > >
> > > +   if (!dev_priv->ggtt.num_fences)
> > > +           return -EOPNOTSUPP;
> > > +
> > >     rcu_read_lock();
> > >     obj = i915_gem_object_lookup_rcu(file, args->handle);
> > >     if (obj) {
> > >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
Jason Ekstrand Aug. 22, 2019, 7:25 p.m. UTC | #4
Acked-by: Jason Ekstrand <jason@jlekstrand.net>

On Wed, Aug 21, 2019 at 10:20 AM Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> On Wed, Aug 21, 2019 at 3:55 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Tue, Aug 20, 2019 at 01:57:44PM -0700, Daniele Ceraolo Spurio wrote:
> > >
> > >
> > > On 8/20/19 12:54 PM, Daniel Vetter wrote:
> > > > The cpu (de)tiler hw is gone, this stopped being useful. Plus it
> never
> > > > supported any of the fancy new tiling formats, which means userspace
> > > > also stopped using the magic side-channel this provides.
> > > >
> > > > This would totally break a lot of the igts, but they're already
> broken
> > > > for the same reasons as userspace on gen12 would be.
> > > >
> > > > v2: Look at ggtt->num_fences instead, that also avoids the need for a
> > > > comment (Chris). This also means that gen12 support really needs to
> > > > make sure num_fences is set to 0. There is a patch for that, but it
> > > > checks for HAS_MAPPABLE_APERTURE, which I'm not sure is the right
> > > > thing really. Adding relevant people.
> > > >
> > >
> > > We'd obviously need to make that setting for all gen12+, because TGL
> > > does have mappable aperture.
> > >
> > > Apart from the tiling ioctl, the only place I see where we set tiling
> is
> > > intel_alloc_initial_plane_obj(), can the users of that object handle
> the
> > > lack of fences gracefully?
> >
> > Gen4+ display engine has its own tiling controls and doesn't care about
> > fences. So we should be able to leave the obj tiling set to NONE.
> >
> > Hmm. Actually I think we should reject tiled framebuffers in the BIOS
> > fb takeover because fbdev needs a linear view for the memory. No can
> > do without the fence.
>
> Yeah I think this is just more fallout from "no more fences in the hw".
> -Daniel
>
> >
> > > When I wrote the num_fences=0 patch I was
> > > expecting display to be unavailable, so I didn't really look at that
> > > part of the code.
> > >
> > > It'd also be nice to be more explicit with fencing since we seem to
> > > often call i915_vma_pin_iomap, which implicitly applies a fence if
> > > needed, on objects that can't be tiled or have had a fence assigned a
> > > few lines before. This is more a nice to have tough, possibly together
> > > with a split of the "mappable" and "fenceable" attributes of the vma.
> > >
> > > Daniele
> > >
> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 7 +++++++
> > > >   1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > > index ca0c2f451742..e5d1ae8d4dba 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > > @@ -313,10 +313,14 @@ int
> > > >   i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
> > > >                       struct drm_file *file)
> > > >   {
> > > > +   struct drm_i915_private *dev_priv = to_i915(dev);
> > > >     struct drm_i915_gem_set_tiling *args = data;
> > > >     struct drm_i915_gem_object *obj;
> > > >     int err;
> > > >
> > > > +   if (!dev_priv->ggtt.num_fences)
> > > > +           return -EOPNOTSUPP;
> > > > +
> > > >     obj = i915_gem_object_lookup(file, args->handle);
> > > >     if (!obj)
> > > >             return -ENOENT;
> > > > @@ -402,6 +406,9 @@ i915_gem_get_tiling_ioctl(struct drm_device
> *dev, void *data,
> > > >     struct drm_i915_gem_object *obj;
> > > >     int err = -ENOENT;
> > > >
> > > > +   if (!dev_priv->ggtt.num_fences)
> > > > +           return -EOPNOTSUPP;
> > > > +
> > > >     rcu_read_lock();
> > > >     obj = i915_gem_object_lookup_rcu(file, args->handle);
> > > >     if (obj) {
> > > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose Aug. 28, 2019, 8:11 p.m. UTC | #5
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

Pushing this one in a few minutes.

On Thu, 2019-08-22 at 14:25 -0500, Jason Ekstrand wrote:
> Acked-by: Jason Ekstrand <jason@jlekstrand.net>
> 
> On Wed, Aug 21, 2019 at 10:20 AM Daniel Vetter <
> daniel.vetter@ffwll.ch> wrote:
> > On Wed, Aug 21, 2019 at 3:55 PM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Tue, Aug 20, 2019 at 01:57:44PM -0700, Daniele Ceraolo Spurio
> > wrote:
> > > >
> > > >
> > > > On 8/20/19 12:54 PM, Daniel Vetter wrote:
> > > > > The cpu (de)tiler hw is gone, this stopped being useful. Plus
> > it never
> > > > > supported any of the fancy new tiling formats, which means
> > userspace
> > > > > also stopped using the magic side-channel this provides.
> > > > >
> > > > > This would totally break a lot of the igts, but they're
> > already broken
> > > > > for the same reasons as userspace on gen12 would be.
> > > > >
> > > > > v2: Look at ggtt->num_fences instead, that also avoids the
> > need for a
> > > > > comment (Chris). This also means that gen12 support really
> > needs to
> > > > > make sure num_fences is set to 0. There is a patch for that,
> > but it
> > > > > checks for HAS_MAPPABLE_APERTURE, which I'm not sure is the
> > right
> > > > > thing really. Adding relevant people.
> > > > >
> > > >
> > > > We'd obviously need to make that setting for all gen12+,
> > because TGL
> > > > does have mappable aperture.
> > > >
> > > > Apart from the tiling ioctl, the only place I see where we set
> > tiling is
> > > > intel_alloc_initial_plane_obj(), can the users of that object
> > handle the
> > > > lack of fences gracefully?
> > >
> > > Gen4+ display engine has its own tiling controls and doesn't care
> > about
> > > fences. So we should be able to leave the obj tiling set to NONE.
> > >
> > > Hmm. Actually I think we should reject tiled framebuffers in the
> > BIOS
> > > fb takeover because fbdev needs a linear view for the memory. No
> > can
> > > do without the fence.
> > 
> > Yeah I think this is just more fallout from "no more fences in the
> > hw".
> > -Daniel
> > 
> > >
> > > > When I wrote the num_fences=0 patch I was
> > > > expecting display to be unavailable, so I didn't really look at
> > that
> > > > part of the code.
> > > >
> > > > It'd also be nice to be more explicit with fencing since we
> > seem to
> > > > often call i915_vma_pin_iomap, which implicitly applies a fence
> > if
> > > > needed, on objects that can't be tiled or have had a fence
> > assigned a
> > > > few lines before. This is more a nice to have tough, possibly
> > together
> > > > with a split of the "mappable" and "fenceable" attributes of
> > the vma.
> > > >
> > > > Daniele
> > > >
> > > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > > > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > > > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 7 +++++++
> > > > >   1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > > > index ca0c2f451742..e5d1ae8d4dba 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > > > > @@ -313,10 +313,14 @@ int
> > > > >   i915_gem_set_tiling_ioctl(struct drm_device *dev, void
> > *data,
> > > > >                       struct drm_file *file)
> > > > >   {
> > > > > +   struct drm_i915_private *dev_priv = to_i915(dev);
> > > > >     struct drm_i915_gem_set_tiling *args = data;
> > > > >     struct drm_i915_gem_object *obj;
> > > > >     int err;
> > > > >
> > > > > +   if (!dev_priv->ggtt.num_fences)
> > > > > +           return -EOPNOTSUPP;
> > > > > +
> > > > >     obj = i915_gem_object_lookup(file, args->handle);
> > > > >     if (!obj)
> > > > >             return -ENOENT;
> > > > > @@ -402,6 +406,9 @@ i915_gem_get_tiling_ioctl(struct
> > drm_device *dev, void *data,
> > > > >     struct drm_i915_gem_object *obj;
> > > > >     int err = -ENOENT;
> > > > >
> > > > > +   if (!dev_priv->ggtt.num_fences)
> > > > > +           return -EOPNOTSUPP;
> > > > > +
> > > > >     rcu_read_lock();
> > > > >     obj = i915_gem_object_lookup_rcu(file, args->handle);
> > > > >     if (obj) {
> > > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > 
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Aug. 28, 2019, 8:13 p.m. UTC | #6
Quoting Souza, Jose (2019-08-28 21:11:53)
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

It's using a non-standard for i915 error code, and get_tiling is not
affected, it will always return LINEAR. You cannot set tiling as there
is no fence (according to the new abi).
-Chris
Souza, Jose Aug. 28, 2019, 8:31 p.m. UTC | #7
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@intel.com>
> 
> It's using a non-standard for i915 error code, and get_tiling is not

Huum should it use ENOTSUPP then?!

> 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.

> there
> is no fence (according to the new abi).
> -Chris
Daniel Vetter Aug. 29, 2019, 6:50 a.m. UTC | #8
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@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.

> > 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
Souza, Jose Sept. 3, 2019, 7:21 p.m. UTC | #9
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@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?

> 
> > > 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 Sept. 4, 2019, 2:29 p.m. UTC | #10
On Tue, Sep 3, 2019 at 9:21 PM Souza, Jose <jose.souza@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@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

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.

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 Sept. 4, 2019, 2:31 p.m. UTC | #11
On Wed, Sep 4, 2019 at 4:29 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Sep 3, 2019 at 9:21 PM Souza, Jose <jose.souza@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@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.
>
> 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
Souza, Jose Sept. 4, 2019, 7:05 p.m. UTC | #12
On Wed, 2019-09-04 at 16:31 +0200, Daniel Vetter wrote:
> On Wed, Sep 4, 2019 at 4:29 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Sep 3, 2019 at 9:21 PM Souza, Jose <jose.souza@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@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@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
> 
>
Daniele Ceraolo Spurio Sept. 19, 2019, 8:57 p.m. UTC | #13
On 8/21/19 8:20 AM, Daniel Vetter wrote:
> On Wed, Aug 21, 2019 at 3:55 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>>
>> On Tue, Aug 20, 2019 at 01:57:44PM -0700, Daniele Ceraolo Spurio wrote:
>>>
>>>
>>> On 8/20/19 12:54 PM, Daniel Vetter wrote:
>>>> The cpu (de)tiler hw is gone, this stopped being useful. Plus it never
>>>> supported any of the fancy new tiling formats, which means userspace
>>>> also stopped using the magic side-channel this provides.
>>>>
>>>> This would totally break a lot of the igts, but they're already broken
>>>> for the same reasons as userspace on gen12 would be.
>>>>
>>>> v2: Look at ggtt->num_fences instead, that also avoids the need for a
>>>> comment (Chris). This also means that gen12 support really needs to
>>>> make sure num_fences is set to 0. There is a patch for that, but it
>>>> checks for HAS_MAPPABLE_APERTURE, which I'm not sure is the right
>>>> thing really. Adding relevant people.
>>>>
>>>
>>> We'd obviously need to make that setting for all gen12+, because TGL
>>> does have mappable aperture.
>>>
>>> Apart from the tiling ioctl, the only place I see where we set tiling is
>>> intel_alloc_initial_plane_obj(), can the users of that object handle the
>>> lack of fences gracefully?
>>
>> Gen4+ display engine has its own tiling controls and doesn't care about
>> fences. So we should be able to leave the obj tiling set to NONE.
>>
>> Hmm. Actually I think we should reject tiled framebuffers in the BIOS
>> fb takeover because fbdev needs a linear view for the memory. No can
>> do without the fence.
> 
> Yeah I think this is just more fallout from "no more fences in the hw".
> -Daniel
> 

Is anyone looking at implementing this (fence = 0 and reject tiled FBs) 
for TGL? I can pick it up if no one is.

Daniele

>>
>>> When I wrote the num_fences=0 patch I was
>>> expecting display to be unavailable, so I didn't really look at that
>>> part of the code.
>>>
>>> It'd also be nice to be more explicit with fencing since we seem to
>>> often call i915_vma_pin_iomap, which implicitly applies a fence if
>>> needed, on objects that can't be tiled or have had a fence assigned a
>>> few lines before. This is more a nice to have tough, possibly together
>>> with a split of the "mappable" and "fenceable" attributes of the vma.
>>>
>>> Daniele
>>>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: Stuart Summers <stuart.summers@intel.com>
>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>> Cc: Kenneth Graunke <kenneth@whitecape.org>
>>>> Cc: Jason Ekstrand <jason@jlekstrand.net>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
>>>> index ca0c2f451742..e5d1ae8d4dba 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
>>>> @@ -313,10 +313,14 @@ int
>>>>    i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
>>>>                        struct drm_file *file)
>>>>    {
>>>> +   struct drm_i915_private *dev_priv = to_i915(dev);
>>>>      struct drm_i915_gem_set_tiling *args = data;
>>>>      struct drm_i915_gem_object *obj;
>>>>      int err;
>>>>
>>>> +   if (!dev_priv->ggtt.num_fences)
>>>> +           return -EOPNOTSUPP;
>>>> +
>>>>      obj = i915_gem_object_lookup(file, args->handle);
>>>>      if (!obj)
>>>>              return -ENOENT;
>>>> @@ -402,6 +406,9 @@ i915_gem_get_tiling_ioctl(struct drm_device *dev, void *data,
>>>>      struct drm_i915_gem_object *obj;
>>>>      int err = -ENOENT;
>>>>
>>>> +   if (!dev_priv->ggtt.num_fences)
>>>> +           return -EOPNOTSUPP;
>>>> +
>>>>      rcu_read_lock();
>>>>      obj = i915_gem_object_lookup_rcu(file, args->handle);
>>>>      if (obj) {
>>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Ville Syrjälä
>> Intel
> 
> 
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
index ca0c2f451742..e5d1ae8d4dba 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
@@ -313,10 +313,14 @@  int
 i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data,
 			  struct drm_file *file)
 {
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_gem_set_tiling *args = data;
 	struct drm_i915_gem_object *obj;
 	int err;
 
+	if (!dev_priv->ggtt.num_fences)
+		return -EOPNOTSUPP;
+
 	obj = i915_gem_object_lookup(file, args->handle);
 	if (!obj)
 		return -ENOENT;
@@ -402,6 +406,9 @@  i915_gem_get_tiling_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_object *obj;
 	int err = -ENOENT;
 
+	if (!dev_priv->ggtt.num_fences)
+		return -EOPNOTSUPP;
+
 	rcu_read_lock();
 	obj = i915_gem_object_lookup_rcu(file, args->handle);
 	if (obj) {