| Message ID | 20190820195451.15671-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: disable set/get_tiling ioctl on gen12+ | expand |
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) { >
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
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
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
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
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
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
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
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
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
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
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 > >
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 > > >
On 8/28/2019 11:50 PM, 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. > >>> 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, I came across usage of GET_TILING in libdrm. Is used in drm_intel_bo_gem_create_from_name() and drm_intel_bo_gem_create_from_prime(). Should these be updated so they don't fail when EOPNOTSUPP is returned on gen12+? Maybe libdrm should just set tiling_mode to 0 on EOPNOTSUPP error instead of those calls failing? -Brian
On Fri, Nov 8, 2019 at 12:07 AM Brian Welty <brian.welty@intel.com> wrote: > > > > On 8/28/2019 11:50 PM, 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. > > > >>> 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, I came across usage of GET_TILING in libdrm. > Is used in drm_intel_bo_gem_create_from_name() and drm_intel_bo_gem_create_from_prime(). > Should these be updated so they don't fail when EOPNOTSUPP is returned on gen12+? > Maybe libdrm should just set tiling_mode to 0 on EOPNOTSUPP error instead of those calls failing? Yes, I guess that part of mesa hasn't seen much testing yet. -Daniel
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) {
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(+)