Re: [PATCH 1/2] drm: Introduce per-device driver_features

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 13, 2018 at 03:50:01PM +0200, Daniel Vetter wrote:
> On Thu, Sep 13, 2018 at 04:16:21PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We wish to control certain driver_features flags on a per-device basis
> > while still sharing a single drm_driver instance across all the
> > devices. To that end introduce device.driver_features. By default
> > it will be set to ~0 to not impose any limits beyond
> > driver.driver_features. Drivers can then clear specific flags
> > in the per-device bitmask to limit the capabilities of the device.
> > 
> > An alternative approach would be to copy the driver_features from
> > the driver into the device in drm_dev_init(), however that would
> > require verifying that no driver is currently changing
> > driver.driver_features after drm_dev_init(). Hence the ~0 apporach
> > was easier.
> > 
> > Ideally we'd also make drm_driver const but there is plenty of code
> > left that wants to mutate it (eg. various vfunc assignments). We'll
> > need to fix all that up before we can make it const.
> > 
> > And while at it fix up the type of the feature flag passed to
> > drm_core_check_feature().
> > 
> > v2: Streamline the && vs. & (Chris)
> >     s/int/u32/ in drm_core_check_feature() args
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> git grep DRIVER_ATOMIC -- drivers/gpu/drm/nouveau has a 2nd supporting
> case for this. Exactly same problem as we have here. Would be good to also
> convert that one, for a bit of OCD.

Thanks for pointing it out. I'll cook it up and send separately after
this lands.

> 
> Irrespective of that, on the series:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Feel free to also add the r-b to the nouveau patch if you do it, it's
> rather obvious.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_drv.c |  3 +++
> >  include/drm/drm_device.h  | 10 ++++++++++
> >  include/drm/drm_drv.h     |  8 ++++----
> >  3 files changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index ea4941da9b27..36e8e9cbec52 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -506,6 +506,9 @@ int drm_dev_init(struct drm_device *dev,
> >  	dev->dev = parent;
> >  	dev->driver = driver;
> >  
> > +	/* no per-device feature limits by default */
> > +	dev->driver_features = ~0u;
> > +
> >  	INIT_LIST_HEAD(&dev->filelist);
> >  	INIT_LIST_HEAD(&dev->filelist_internal);
> >  	INIT_LIST_HEAD(&dev->clientlist);
> > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> > index f9c6e0e3aec7..42411b3ea0c8 100644
> > --- a/include/drm/drm_device.h
> > +++ b/include/drm/drm_device.h
> > @@ -45,6 +45,16 @@ struct drm_device {
> >  	/* currently active master for this device. Protected by master_mutex */
> >  	struct drm_master *master;
> >  
> > +	/**
> > +	 * @driver_features: per-device driver features
> > +	 *
> > +	 * Drivers can clear specific flags here to disallow
> > +	 * certain features on a per-device basis while still
> > +	 * sharing a single &struct drm_driver instance across
> > +	 * all devices.
> > +	 */
> > +	u32 driver_features;
> > +
> >  	/**
> >  	 * @unplugged:
> >  	 *
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index 23b9678137a6..8830e3de3a86 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -653,14 +653,14 @@ static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> >   * @dev: DRM device to check
> >   * @feature: feature flag
> >   *
> > - * This checks @dev for driver features, see &drm_driver.driver_features and the
> > - * various DRIVER_\* flags.
> > + * This checks @dev for driver features, see &drm_driver.driver_features,
> > + * &drm_device.driver_features, and the various DRIVER_\* flags.
> >   *
> >   * Returns true if the @feature is supported, false otherwise.
> >   */
> > -static inline bool drm_core_check_feature(struct drm_device *dev, int feature)
> > +static inline bool drm_core_check_feature(struct drm_device *dev, u32 feature)
> >  {
> > -	return dev->driver->driver_features & feature;
> > +	return dev->driver->driver_features & dev->driver_features & feature;
> >  }
> >  
> >  /**
> > -- 
> > 2.16.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx