| Message ID | 1351272650-8402-1-git-send-email-damien.lespiau@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 26 Oct 2012 18:30:50 +0100 Damien Lespiau <damien.lespiau@gmail.com> wrote: > From: Damien Lespiau <damien.lespiau@intel.com> > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 3434b6e..aa8d09b 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -465,6 +465,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > if (intel_plane->pipe != intel_crtc->pipe) > return -EINVAL; > > + /* Sprite planes can be linear or x-tiled surfaces */ > + if (!(obj->tiling_mode == I915_TILING_NONE || > + obj->tiling_mode == I915_TILING_X)) > + return -EINVAL; > + > /* > * Clamp the width & height into the visible area. Note we don't > * try to scale the source if part of the visible region is offscreen. Do we not catch this when we make the fb? If not we may want to do this there...
On Fri, 26 Oct 2012 10:42:54 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Fri, 26 Oct 2012 18:30:50 +0100 > Damien Lespiau <damien.lespiau@gmail.com> wrote: > > > From: Damien Lespiau <damien.lespiau@intel.com> > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index 3434b6e..aa8d09b 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -465,6 +465,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > if (intel_plane->pipe != intel_crtc->pipe) > > return -EINVAL; > > > > + /* Sprite planes can be linear or x-tiled surfaces */ > > + if (!(obj->tiling_mode == I915_TILING_NONE || > > + obj->tiling_mode == I915_TILING_X)) > > + return -EINVAL; > > + > > /* > > * Clamp the width & height into the visible area. Note we don't > > * try to scale the source if part of the visible region is offscreen. > > Do we not catch this when we make the fb? If not we may want to do > this there... We do. However, we don't catch userspace changing the tiling on an existing fb (unless actually pinned)... The extra level of defense is certainly welcome, and provides documentation at all levels on the known capabilities of the hw. (The only downside is that we have more guards to fixup if those ever change, but unlikely ;) The only bikeshed is that elsewhere we use switch (obj->tiling_mode) { case I915_TILING_NONE: case I915_TILING_X: break; default: return -EINVAL; } Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Sun, Oct 28, 2012 at 10:04 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > We do. However, we don't catch userspace changing the tiling on an > existing fb (unless actually pinned)... The extra level of defense is > certainly welcome, and provides documentation at all levels on the known > capabilities of the hw. (The only downside is that we have more guards to > fixup if those ever change, but unlikely ;) I've sent a new version of this one then, turns out it be useful to catch cases where one changes the tiling mode after the creation of the fb.
On Sun, Oct 28, 2012 at 10:04:54AM +0000, Chris Wilson wrote: > On Fri, 26 Oct 2012 10:42:54 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Fri, 26 Oct 2012 18:30:50 +0100 > > Damien Lespiau <damien.lespiau@gmail.com> wrote: > > > > > From: Damien Lespiau <damien.lespiau@intel.com> > > > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_sprite.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > index 3434b6e..aa8d09b 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -465,6 +465,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > if (intel_plane->pipe != intel_crtc->pipe) > > > return -EINVAL; > > > > > > + /* Sprite planes can be linear or x-tiled surfaces */ > > > + if (!(obj->tiling_mode == I915_TILING_NONE || > > > + obj->tiling_mode == I915_TILING_X)) > > > + return -EINVAL; > > > + > > > /* > > > * Clamp the width & height into the visible area. Note we don't > > > * try to scale the source if part of the visible region is offscreen. > > > > Do we not catch this when we make the fb? If not we may want to do > > this there... > > We do. However, we don't catch userspace changing the tiling on an > existing fb (unless actually pinned)... Why not simply count how many fbs reference a given bo, and reject tiling changes when the count > 0?
On Mon, 29 Oct 2012 15:20:15 +0000 "Lespiau, Damien" <damien.lespiau@intel.com> wrote: > On Sun, Oct 28, 2012 at 10:04 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > We do. However, we don't catch userspace changing the tiling on an > > existing fb (unless actually pinned)... The extra level of defense is > > certainly welcome, and provides documentation at all levels on the known > > capabilities of the hw. (The only downside is that we have more guards to > > fixup if those ever change, but unlikely ;) > > I've sent a new version of this one then, turns out it be useful to > catch cases where one changes the tiling mode after the creation of > the fb. > Ok sounds good. I'll add my r-b to the latest one.
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 3434b6e..aa8d09b 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -465,6 +465,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (intel_plane->pipe != intel_crtc->pipe) return -EINVAL; + /* Sprite planes can be linear or x-tiled surfaces */ + if (!(obj->tiling_mode == I915_TILING_NONE || + obj->tiling_mode == I915_TILING_X)) + return -EINVAL; + /* * Clamp the width & height into the visible area. Note we don't * try to scale the source if part of the visible region is offscreen.