| Message ID | 20161130083002.1520-2-michel@daenzer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 30, 2016 at 05:30:02PM +0900, Michel Dänzer wrote: > From: Michel Dänzer <michel.daenzer@amd.com> > > This is an attempt to make the previous fix a bit more robust going > forward. > > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com> > --- > drivers/gpu/drm/drm_ioctl.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 71c3473..32f484b 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -229,6 +229,19 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ > struct drm_crtc *crtc; > > req->value = 0; > + > + /* Only allow non-KMS caps with non-KMS drivers */ > + switch (req->capability) { > + case DRM_CAP_DUMB_BUFFER: Dumb buffers are only meant to be used for kms drivers, should be disallowed too. > + case DRM_CAP_VBLANK_HIGH_CRTC: Might be good to have a comment here that we need to allow this for old ums? > + case DRM_CAP_PRIME: > + case DRM_CAP_TIMESTAMP_MONOTONIC: This is pretty new, I don't think any of the old ums drivers was ever updated to use it. Should probably disallow it too. > + break; > + default: > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) > + return -ENOTSUPP; > + } And one code org bikeshed: I don't like the duplicated switch, could we instead split it it into two disjoint sets like this? switch (req->capability) { case DRM_CAP_PRIME: req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0; req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0; break; ... all other non-modeset caps ... } if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -ENOTSUPP; switch (req->capability) { ... handle remaining caps needed for DRIVER_MODSET ... default: return -EINVAL; } That way it would be a bit more obvious that people who add a new cap need to make a decision where to put it (and by default put it in the bottom pile). -Daniel > switch (req->capability) { > case DRM_CAP_DUMB_BUFFER: > if (dev->driver->dumb_create) > @@ -254,12 +267,10 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ > req->value = dev->mode_config.async_page_flip; > break; > case DRM_CAP_PAGE_FLIP_TARGET: > - if (drm_core_check_feature(dev, DRIVER_MODESET)) { > - req->value = 1; > - drm_for_each_crtc(crtc, dev) { > - if (!crtc->funcs->page_flip_target) > - req->value = 0; > - } > + req->value = 1; > + drm_for_each_crtc(crtc, dev) { > + if (!crtc->funcs->page_flip_target) > + req->value = 0; > } > break; > case DRM_CAP_CURSOR_WIDTH: > -- > 2.10.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Nov 30, 2016 at 4:07 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Nov 30, 2016 at 05:30:02PM +0900, Michel Dänzer wrote: >> From: Michel Dänzer <michel.daenzer@amd.com> >> >> This is an attempt to make the previous fix a bit more robust going >> forward. >> >> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com> >> --- >> drivers/gpu/drm/drm_ioctl.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index 71c3473..32f484b 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -229,6 +229,19 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ >> struct drm_crtc *crtc; >> >> req->value = 0; >> + >> + /* Only allow non-KMS caps with non-KMS drivers */ >> + switch (req->capability) { >> + case DRM_CAP_DUMB_BUFFER: > > Dumb buffers are only meant to be used for kms drivers, should be > disallowed too. > >> + case DRM_CAP_VBLANK_HIGH_CRTC: > > Might be good to have a comment here that we need to allow this for old > ums? > I don't think we need this for UMS. It was added for evergreen and we only supported this feature on KMS. Alex >> + case DRM_CAP_PRIME: >> + case DRM_CAP_TIMESTAMP_MONOTONIC: > > This is pretty new, I don't think any of the old ums drivers was ever > updated to use it. Should probably disallow it too. >> + break; >> + default: >> + if (!drm_core_check_feature(dev, DRIVER_MODESET)) >> + return -ENOTSUPP; >> + } > > And one code org bikeshed: I don't like the duplicated switch, could we > instead split it it into two disjoint sets like this? > > switch (req->capability) { > case DRM_CAP_PRIME: > req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0; > req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0; > break; > ... all other non-modeset caps ... > } > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -ENOTSUPP; > > switch (req->capability) { > ... handle remaining caps needed for DRIVER_MODSET ... > default: > return -EINVAL; > } > > That way it would be a bit more obvious that people who add a new cap need > to make a decision where to put it (and by default put it in the bottom > pile). > -Daniel > >> switch (req->capability) { >> case DRM_CAP_DUMB_BUFFER: >> if (dev->driver->dumb_create) >> @@ -254,12 +267,10 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ >> req->value = dev->mode_config.async_page_flip; >> break; >> case DRM_CAP_PAGE_FLIP_TARGET: >> - if (drm_core_check_feature(dev, DRIVER_MODESET)) { >> - req->value = 1; >> - drm_for_each_crtc(crtc, dev) { >> - if (!crtc->funcs->page_flip_target) >> - req->value = 0; >> - } >> + req->value = 1; >> + drm_for_each_crtc(crtc, dev) { >> + if (!crtc->funcs->page_flip_target) >> + req->value = 0; >> } >> break; >> case DRM_CAP_CURSOR_WIDTH: >> -- >> 2.10.2 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 30/11/16 06:07 PM, Daniel Vetter wrote: > On Wed, Nov 30, 2016 at 05:30:02PM +0900, Michel Dänzer wrote: >> From: Michel Dänzer <michel.daenzer@amd.com> >> >> This is an attempt to make the previous fix a bit more robust going >> forward. >> >> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com> >> --- >> drivers/gpu/drm/drm_ioctl.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index 71c3473..32f484b 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -229,6 +229,19 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ >> struct drm_crtc *crtc; >> >> req->value = 0; >> + >> + /* Only allow non-KMS caps with non-KMS drivers */ >> + switch (req->capability) { >> + case DRM_CAP_DUMB_BUFFER: > > Dumb buffers are only meant to be used for kms drivers, should be > disallowed too. > >> + case DRM_CAP_VBLANK_HIGH_CRTC: > > Might be good to have a comment here that we need to allow this for old > ums? This is effectively KMS-only as well, per Alex (thanks!). >> + case DRM_CAP_PRIME: >> + case DRM_CAP_TIMESTAMP_MONOTONIC: > > This is pretty new, I don't think any of the old ums drivers was ever > updated to use it. Should probably disallow it too. DRM_CAP_TIMESTAMP_MONOTONIC is driver independent, I don't see why it wouldn't work fine with UMS drivers. OTOH, I don't think DRM_CAP_PRIME can work with UMS. >> + break; >> + default: >> + if (!drm_core_check_feature(dev, DRIVER_MODESET)) >> + return -ENOTSUPP; >> + } > > And one code org bikeshed: I don't like the duplicated switch, could we > instead split it it into two disjoint sets like this? > > switch (req->capability) { > case DRM_CAP_PRIME: > req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0; > req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0; > break; > ... all other non-modeset caps ... > } > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -ENOTSUPP; > > switch (req->capability) { > ... handle remaining caps needed for DRIVER_MODSET ... > default: > return -EINVAL; > } > > That way it would be a bit more obvious that people who add a new cap need > to make a decision where to put it (and by default put it in the bottom > pile). Your pseudo-code wouldn't work correctly, but I get your idea. :) v2 addressing review feedback coming up soon.
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 71c3473..32f484b 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -229,6 +229,19 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ struct drm_crtc *crtc; req->value = 0; + + /* Only allow non-KMS caps with non-KMS drivers */ + switch (req->capability) { + case DRM_CAP_DUMB_BUFFER: + case DRM_CAP_VBLANK_HIGH_CRTC: + case DRM_CAP_PRIME: + case DRM_CAP_TIMESTAMP_MONOTONIC: + break; + default: + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -ENOTSUPP; + } + switch (req->capability) { case DRM_CAP_DUMB_BUFFER: if (dev->driver->dumb_create) @@ -254,12 +267,10 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ req->value = dev->mode_config.async_page_flip; break; case DRM_CAP_PAGE_FLIP_TARGET: - if (drm_core_check_feature(dev, DRIVER_MODESET)) { - req->value = 1; - drm_for_each_crtc(crtc, dev) { - if (!crtc->funcs->page_flip_target) - req->value = 0; - } + req->value = 1; + drm_for_each_crtc(crtc, dev) { + if (!crtc->funcs->page_flip_target) + req->value = 0; } break; case DRM_CAP_CURSOR_WIDTH: