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.syrjala at linux.intel.com] > > Sent: Monday, April 27, 2015 6:04 PM > > To: Antoine, Peter > > Cc: intel-gfx at lists.freedesktop.org; airlied at redhat.com; dri-devel at > > lists.freedesktop.org; daniel.vetter at 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.antoine at 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?
