[RFC] drm/i915: Restrict legacy color key ioctl to pre-gen12
diff mbox series

Message ID 20200114224508.3302967-1-matthew.d.roper@intel.com
State New
Headers show
Series
  • [RFC] drm/i915: Restrict legacy color key ioctl to pre-gen12
Related show

Commit Message

Matt Roper Jan. 14, 2020, 10:45 p.m. UTC
Since gen12 platform support isn't finalized yet, let's kill off the
legacy color key ioctl for this platform; there's no userspace today
that can run on this platform that utilizes this legacy ioctl, so we can
safely kill it now before it becomes ABI.

Color key functionality never got integrated into the property / atomic
interface, and the only known open source consumer was the Intel DDX
which was never updated to run on platforms beyond gen9.  If color
keying is desired going forward, it should really be exposed as a
property so that it can be applied atomically with other display updates
(and should probably be standardized in a way all drivers can choose to
support rather than being i915-specific).

Arguably we might be able to prohibit this on gen10 and gen11 as well
since no open source userspace exists for those platforms that utilizes
these ioctls.  However there's always the very slight chance that
unknown closed source software is actively utilizing the color key ioctl
on those platforms, so we should maintain the support there to avoid
breaking ABI.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/display/intel_sprite.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Chris Wilson Jan. 14, 2020, 10:47 p.m. UTC | #1
Quoting Matt Roper (2020-01-14 22:45:08)
> Since gen12 platform support isn't finalized yet, let's kill off the
> legacy color key ioctl for this platform; there's no userspace today
> that can run on this platform that utilizes this legacy ioctl, so we can
> safely kill it now before it becomes ABI.
> 
> Color key functionality never got integrated into the property / atomic
> interface, and the only known open source consumer was the Intel DDX
> which was never updated to run on platforms beyond gen9.  If color
> keying is desired going forward, it should really be exposed as a
> property so that it can be applied atomically with other display updates
> (and should probably be standardized in a way all drivers can choose to
> support rather than being i915-specific).

But it does run on those platforms and exposes the sprite plane via Xv.
-Chris
Chris Wilson Jan. 14, 2020, 10:54 p.m. UTC | #2
Quoting Chris Wilson (2020-01-14 22:47:58)
> Quoting Matt Roper (2020-01-14 22:45:08)
> > Since gen12 platform support isn't finalized yet, let's kill off the
> > legacy color key ioctl for this platform; there's no userspace today
> > that can run on this platform that utilizes this legacy ioctl, so we can
> > safely kill it now before it becomes ABI.
> > 
> > Color key functionality never got integrated into the property / atomic
> > interface, and the only known open source consumer was the Intel DDX
> > which was never updated to run on platforms beyond gen9.  If color
> > keying is desired going forward, it should really be exposed as a
> > property so that it can be applied atomically with other display updates
> > (and should probably be standardized in a way all drivers can choose to
> > support rather than being i915-specific).
> 
> But it does run on those platforms and exposes the sprite plane via Xv.

What you can say is that the color_key is not required by the Xv sprite
code. We will just unfortunately advertise it to clients, but not
actually do anything.
-Chris
Matt Roper Jan. 14, 2020, 11:03 p.m. UTC | #3
On Tue, Jan 14, 2020 at 10:54:50PM +0000, Chris Wilson wrote:
> Quoting Chris Wilson (2020-01-14 22:47:58)
> > Quoting Matt Roper (2020-01-14 22:45:08)
> > > Since gen12 platform support isn't finalized yet, let's kill off the
> > > legacy color key ioctl for this platform; there's no userspace today
> > > that can run on this platform that utilizes this legacy ioctl, so we can
> > > safely kill it now before it becomes ABI.
> > > 
> > > Color key functionality never got integrated into the property / atomic
> > > interface, and the only known open source consumer was the Intel DDX
> > > which was never updated to run on platforms beyond gen9.  If color
> > > keying is desired going forward, it should really be exposed as a
> > > property so that it can be applied atomically with other display updates
> > > (and should probably be standardized in a way all drivers can choose to
> > > support rather than being i915-specific).
> > 
> > But it does run on those platforms and exposes the sprite plane via Xv.
> 

Hmm, looks like I overlooked 00184dc03 ("Sync i915_pciids upto
d0e062ebb3a4") which finally brought in the CNL and ICL pci ids.  As far
as I can see it still lacks EHL/JSL and TGL as far as I can see.  Are
there plans to continue support for xf86-video-intel for future
platforms?  If not, then cutting the support off at TGL would still be
safe.


Matt

> What you can say is that the color_key is not required by the Xv sprite
> code. We will just unfortunately advertise it to clients, but not
> actually do anything.
> -Chris
Chris Wilson Jan. 14, 2020, 11:07 p.m. UTC | #4
Quoting Matt Roper (2020-01-14 23:03:20)
> On Tue, Jan 14, 2020 at 10:54:50PM +0000, Chris Wilson wrote:
> > Quoting Chris Wilson (2020-01-14 22:47:58)
> > > Quoting Matt Roper (2020-01-14 22:45:08)
> > > > Since gen12 platform support isn't finalized yet, let's kill off the
> > > > legacy color key ioctl for this platform; there's no userspace today
> > > > that can run on this platform that utilizes this legacy ioctl, so we can
> > > > safely kill it now before it becomes ABI.
> > > > 
> > > > Color key functionality never got integrated into the property / atomic
> > > > interface, and the only known open source consumer was the Intel DDX
> > > > which was never updated to run on platforms beyond gen9.  If color
> > > > keying is desired going forward, it should really be exposed as a
> > > > property so that it can be applied atomically with other display updates
> > > > (and should probably be standardized in a way all drivers can choose to
> > > > support rather than being i915-specific).
> > > 
> > > But it does run on those platforms and exposes the sprite plane via Xv.
> > 
> 
> Hmm, looks like I overlooked 00184dc03 ("Sync i915_pciids upto
> d0e062ebb3a4") which finally brought in the CNL and ICL pci ids.  As far
> as I can see it still lacks EHL/JSL and TGL as far as I can see.  Are
> there plans to continue support for xf86-video-intel for future
> platforms?  If not, then cutting the support off at TGL would still be
> safe.

It uses a generic binding to 0x8086. That it provides sprites planes is
one of the reasons why it is invaluable.
-Chris
Ville Syrjälä Jan. 15, 2020, 2:31 p.m. UTC | #5
On Tue, Jan 14, 2020 at 02:45:08PM -0800, Matt Roper wrote:
> Since gen12 platform support isn't finalized yet, let's kill off the
> legacy color key ioctl for this platform; there's no userspace today
> that can run on this platform that utilizes this legacy ioctl, so we can
> safely kill it now before it becomes ABI.
> 
> Color key functionality never got integrated into the property / atomic
> interface, and the only known open source consumer was the Intel DDX
> which was never updated to run on platforms beyond gen9.  If color
> keying is desired going forward, it should really be exposed as a
> property so that it can be applied atomically with other display updates
> (and should probably be standardized in a way all drivers can choose to
> support rather than being i915-specific).
> 
> Arguably we might be able to prohibit this on gen10 and gen11 as well
> since no open source userspace exists for those platforms that utilizes
> these ioctls.  However there's always the very slight chance that
> unknown closed source software is actively utilizing the color key ioctl
> on those platforms, so we should maintain the support there to avoid
> breaking ABI.

Can't really see much point in this. The hardware hasn't changed so
arbitrarily cutting this off won't simplify anything.

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_sprite.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index fca77ec1e0dd..6e8a4686a406 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -2290,6 +2290,14 @@ int intel_sprite_set_colorkey_ioctl(struct drm_device *dev, void *data,
>  	struct drm_modeset_acquire_ctx ctx;
>  	int ret = 0;
>  
> +	/*
> +	 * Userspace that uses this legacy interface only exists up through
> +	 * gen9.  Discontinue support for the interface starting with gen12 so
> +	 * that it doesn't become ABI on newer platforms.
> +	 */
> +	if (INTEL_GEN(dev_priv) >= 12)
> +		return -EINVAL;
> +
>  	/* ignore the pointless "none" flag */
>  	set->flags &= ~I915_SET_COLORKEY_NONE;
>  
> -- 
> 2.23.0

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index fca77ec1e0dd..6e8a4686a406 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -2290,6 +2290,14 @@  int intel_sprite_set_colorkey_ioctl(struct drm_device *dev, void *data,
 	struct drm_modeset_acquire_ctx ctx;
 	int ret = 0;
 
+	/*
+	 * Userspace that uses this legacy interface only exists up through
+	 * gen9.  Discontinue support for the interface starting with gen12 so
+	 * that it doesn't become ABI on newer platforms.
+	 */
+	if (INTEL_GEN(dev_priv) >= 12)
+		return -EINVAL;
+
 	/* ignore the pointless "none" flag */
 	set->flags &= ~I915_SET_COLORKEY_NONE;