| Message ID | 20180913131622.17690-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm: Introduce per-device driver_features | expand |
Quoting Ville Syrjala (2018-09-13 14:16:21) > 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> The gradual transition makes sense (less work!), as does being able to deselect features on individual devices. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
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. 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
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
On 2018-09-13 4:29 p.m., Ville Syrjälä wrote: > 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. I don't suppose you'd like to do amdgpu as well, while you're at it? :) Either way, this is awesome stuff! This patch is Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
On Thu, Sep 13, 2018 at 04:52:34PM +0200, Michel Dänzer wrote: > On 2018-09-13 4:29 p.m., Ville Syrjälä wrote: > > 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. > > I don't suppose you'd like to do amdgpu as well, while you're at it? :) Sure. I'll take a gander at it as well.
On Thu, Sep 13, 2018 at 06:06:01PM +0300, Ville Syrjälä wrote: > On Thu, Sep 13, 2018 at 04:52:34PM +0200, Michel Dänzer wrote: > > On 2018-09-13 4:29 p.m., Ville Syrjälä wrote: > > > 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. > > > > I don't suppose you'd like to do amdgpu as well, while you're at it? :) > > Sure. I'll take a gander at it as well. Series pushed to drm-misc-next. Thanks for the reviews.
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; } /**