reply at end.
On Tue, 2015-04-28 at 13:40 +0300, Ville Syrjälä wrote:
> On Tue, Apr 28, 2015 at 05:52:20AM +0000, Antoine, Peter wrote:
> > Hi,
> >
> > (replies inline)
> >
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> > Sent: Monday, April 27, 2015 6:04 PM
> > To: Antoine, Peter
> > Cc: intel-gfx@lists.freedesktop.org; airl...@redhat.com;
> > dri-de...@lists.freedesktop.org; daniel.vet...@ffwll.ch
> > Subject: Re: [Intel-gfx] [PATCH 4/5] drm: Make HW_LOCK access functions
> > optional.
> >
> > On Thu, Apr 23, 2015 at 03:07:57PM +0100, Peter Antoine wrote:
> > > As these functions are only used by one driver and there are security
> > > holes in these functions. Make the functions optional.
> > >
> > > Issue: VIZ-5485
> > > Signed-off-by: Peter Antoine <peter.anto...@intel.com>
> > > ---
> > > drivers/gpu/drm/drm_lock.c | 6 ++++++
> > > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > > drivers/gpu/drm/nouveau/nouveau_drm.c | 3 ++-
> > > include/drm/drmP.h | 23 ++++++++++++-----------
> > > include/uapi/drm/i915_drm.h | 1 +
> > > 5 files changed, 24 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > > index 94500930..b61d4c7 100644
> > > --- a/drivers/gpu/drm/drm_lock.c
> > > +++ b/drivers/gpu/drm/drm_lock.c
> > > @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> > > struct drm_master *master = file_priv->master;
> > > int ret = 0;
> > >
> > > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > > + return -EINVAL;
> > > +
> > > ++file_priv->lock_count;
> > >
> > > if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) { @@
> > > -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void
> > > *data, struct drm_file *file_
> > > struct drm_lock *lock = data;
> > > struct drm_master *master = file_priv->master;
> > >
> > > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > > + return -EINVAL;
> > > +
> > > if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> > > DRM_ERROR("Process %d using kernel context %d\n",
> > > task_pid_nr(current), lock->context); diff --git
> > > a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index e44116f..c771ef0 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void
> > > *data,
> > > if (!value)
> > > return -ENODEV;
> > > break;
> > > + case I915_PARAM_HAS_LEGACY_CONTEXT:
> > > + value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> > > + break;
> >
> > Seems pointless to add a parameter that'll always be false.
> >
> > There is some history to these changes, the HW_LOCK functions were removed
> > previously but causes an issue with the Nouveau drivers. So that the
> > functions where put back in. So the parameter has been added to allow for
> > that driver to turn the legacy context on as it is needed.
> >
> > > default:
> > > DRM_DEBUG("Unknown parameter %d\n", param->param);
> > > return -EINVAL;
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > index 8763deb..936b423 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > @@ -940,7 +940,8 @@ static struct drm_driver driver_stub = {
> > > .driver_features =
> > > DRIVER_USE_AGP |
> > > - DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> > > + DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> > > + DRIVER_KMS_LEGACY_CONTEXT,
> >
> > Why is this here? AFAICS only the via driver cares about legacy contexts,
> > and only dri1 drivers care about the hw lock.
> >
> > See above.
> > >
> > > .load = nouveau_drm_load,
> > > .unload = nouveau_drm_unload,
> > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h index
> > > 62c40777..367e42f 100644
> > > --- a/include/drm/drmP.h
> > > +++ b/include/drm/drmP.h
> > > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...); /*@{*/
> > >
> > > /* driver capabilities and requirements mask */
> > > -#define DRIVER_USE_AGP 0x1
> > > -#define DRIVER_PCI_DMA 0x8
> > > -#define DRIVER_SG 0x10
> > > -#define DRIVER_HAVE_DMA 0x20
> > > -#define DRIVER_HAVE_IRQ 0x40
> > > -#define DRIVER_IRQ_SHARED 0x80
> > > -#define DRIVER_GEM 0x1000
> > > -#define DRIVER_MODESET 0x2000
> > > -#define DRIVER_PRIME 0x4000
> > > -#define DRIVER_RENDER 0x8000
> > > -#define DRIVER_ATOMIC 0x10000
> > > +#define DRIVER_USE_AGP 0x1
> > > +#define DRIVER_PCI_DMA 0x8
> > > +#define DRIVER_SG 0x10
> > > +#define DRIVER_HAVE_DMA 0x20
> > > +#define DRIVER_HAVE_IRQ 0x40
> > > +#define DRIVER_IRQ_SHARED 0x80
> > > +#define DRIVER_GEM 0x1000
> > > +#define DRIVER_MODESET 0x2000
> > > +#define DRIVER_PRIME 0x4000
> > > +#define DRIVER_RENDER 0x8000
> > > +#define DRIVER_ATOMIC 0x10000
> > > +#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
> >
> > Why is there KMS in the name?
> >
> > By suggestion of Daniel.
> >
> > I was thinking just checking for GEM, but I think there was some
> > gem+dri1 userland for i915 at some point in time. ums and dri1 are
> > now dead as far as i915 is concerned, so in theory it should be fine.
> > But I'm not sure if some other driver might have the same baggage.
> >
> > Other drivers have the same baggage.
> >
> > I suppose one option would be to check for MODESET instead. kms+dri1
> > doesn't sound like an entirely sane combination to me.
> >
> > Can't use the MODESET as this was how it was turned off in the previous
> > incarnation and was reverted by Dave Airle.
>
> Reference?
From the next commit [5/5] as it is the one that actually turns off the
functions that were turned off before.
These changes are based on the two patches:
commit c21eb21cb50d58e7cbdcb8b9e7ff68b85cfa5095
Author: Dave Airlie <airl...@redhat.com>
And the commit that the above patch reverts:
commit 7c510133d93dd6f15ca040733ba7b2891ed61fd1
Author: Daniel Vetter <daniel.vet...@ffwll.ch>
>
> >
> > Peter.
> >
> > >
> > >
> > > /*********************************************************************
> > > **/
> > > /** \name Macros to make printk easier */ diff --git
> > > a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index
> > > 4851d66..0ad611a2 100644
> > > --- a/include/uapi/drm/i915_drm.h
> > > +++ b/include/uapi/drm/i915_drm.h
> > > @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait {
> > > #define I915_PARAM_REVISION 32
> > > #define I915_PARAM_SUBSLICE_TOTAL 33
> > > #define I915_PARAM_EU_TOTAL 34
> > > +#define I915_PARAM_HAS_LEGACY_CONTEXT 35
> > >
> > > typedef struct drm_i915_getparam {
> > > int param;
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
