| Message ID | 20180913221341.25396-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Return -ENOTSUPP in drm_setclientcap() when driver do not support KMS | expand |
Quoting José Roberto de Souza (2018-09-13 23:13:41) > All DRM_CLIENT capabilities are tied to KMS support, so returning > -ENOTSUPP when KMS is not supported. The posix errno is ENOTSUP (ENOTSUPP is internal). Now since we have no ENOTSUP in the uapi, I've switched to using EOPNOTSUP as that is documented to have the same value as ENOTSUP under Linux. (At least until somebody decided to make ENOTSUP unique.) > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/drm_ioctl.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 6b4a633b4240..842423fe9762 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -306,6 +306,9 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) > { > struct drm_set_client_cap *req = data; > > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) > + return -ENOTSUPP; The wider question though is client cap restricted to modesetting capabilities? Or should each case include a check like DRM_CLIENT_CAP_ATOMIC. -Chris
On Fri, 2018-09-14 at 09:15 +0100, Chris Wilson wrote: > Quoting José Roberto de Souza (2018-09-13 23:13:41) > > All DRM_CLIENT capabilities are tied to KMS support, so returning > > -ENOTSUPP when KMS is not supported. > > The posix errno is ENOTSUP (ENOTSUPP is internal). Now since we have > no > ENOTSUP in the uapi, I've switched to using EOPNOTSUP as that is > documented to have the same value as ENOTSUP under Linux. (At least > until somebody decided to make ENOTSUP unique.) Oh thanks, I have copied it from drm_getcap() and did not notice. > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/drm_ioctl.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_ioctl.c > > b/drivers/gpu/drm/drm_ioctl.c > > index 6b4a633b4240..842423fe9762 100644 > > --- a/drivers/gpu/drm/drm_ioctl.c > > +++ b/drivers/gpu/drm/drm_ioctl.c > > @@ -306,6 +306,9 @@ drm_setclientcap(struct drm_device *dev, void > > *data, struct drm_file *file_priv) > > { > > struct drm_set_client_cap *req = data; > > > > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) > > + return -ENOTSUPP; > > The wider question though is client cap restricted to modesetting > capabilities? Or should each case include a check like > DRM_CLIENT_CAP_ATOMIC. Well all of those: DRM_CLIENT_CAP_STEREO_3D DRM_CLIENT_CAP_UNIVERSAL_PLANES DRM_CLIENT_CAP_ATOMIC DRM_CLIENT_CAP_ASPECT_RATIO DRM_CLIENT_CAP_WRITEBACK_CONNECTORS are just usefull with KMS. > -Chris
Quoting Souza, Jose (2018-09-14 17:30:59) > On Fri, 2018-09-14 at 09:15 +0100, Chris Wilson wrote: > > Quoting José Roberto de Souza (2018-09-13 23:13:41) > > > @@ -306,6 +306,9 @@ drm_setclientcap(struct drm_device *dev, void > > > *data, struct drm_file *file_priv) > > > { > > > struct drm_set_client_cap *req = data; > > > > > > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) > > > + return -ENOTSUPP; > > > > The wider question though is client cap restricted to modesetting > > capabilities? Or should each case include a check like > > DRM_CLIENT_CAP_ATOMIC. > > Well all of those: > > DRM_CLIENT_CAP_STEREO_3D > DRM_CLIENT_CAP_UNIVERSAL_PLANES > DRM_CLIENT_CAP_ATOMIC > DRM_CLIENT_CAP_ASPECT_RATIO > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS > > are just usefull with KMS. It more about the semantics. If it's the first guard in the function, it gives the impression that the setclientcap ioctl is KMS only. If they are repeated for each case, then it's clear that the ioctl is more general and it just those caps that are KMS only. Imo, the drm_client is wider than the kms interface, but I may be wrong. -Chris
On Fri, Sep 14, 2018 at 7:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Souza, Jose (2018-09-14 17:30:59) >> On Fri, 2018-09-14 at 09:15 +0100, Chris Wilson wrote: >> > Quoting José Roberto de Souza (2018-09-13 23:13:41) >> > > @@ -306,6 +306,9 @@ drm_setclientcap(struct drm_device *dev, void >> > > *data, struct drm_file *file_priv) >> > > { >> > > struct drm_set_client_cap *req = data; >> > > >> > > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) >> > > + return -ENOTSUPP; >> > >> > The wider question though is client cap restricted to modesetting >> > capabilities? Or should each case include a check like >> > DRM_CLIENT_CAP_ATOMIC. >> >> Well all of those: >> >> DRM_CLIENT_CAP_STEREO_3D >> DRM_CLIENT_CAP_UNIVERSAL_PLANES >> DRM_CLIENT_CAP_ATOMIC >> DRM_CLIENT_CAP_ASPECT_RATIO >> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS >> >> are just usefull with KMS. > > It more about the semantics. If it's the first guard in the function, it > gives the impression that the setclientcap ioctl is KMS only. If they > are repeated for each case, then it's clear that the ioctl is more > general and it just those caps that are KMS only. > > Imo, the drm_client is wider than the kms interface, but I may be wrong. In getcap we have 2 blocks, with DRIVER_MODESET check in between. I think a comment along the lines of > -Chris > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Sep 14, 2018 at 10:02 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Sep 14, 2018 at 7:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> Quoting Souza, Jose (2018-09-14 17:30:59) >>> On Fri, 2018-09-14 at 09:15 +0100, Chris Wilson wrote: >>> > Quoting José Roberto de Souza (2018-09-13 23:13:41) >>> > > @@ -306,6 +306,9 @@ drm_setclientcap(struct drm_device *dev, void >>> > > *data, struct drm_file *file_priv) >>> > > { >>> > > struct drm_set_client_cap *req = data; >>> > > >>> > > + if (!drm_core_check_feature(dev, DRIVER_MODESET)) >>> > > + return -ENOTSUPP; >>> > >>> > The wider question though is client cap restricted to modesetting >>> > capabilities? Or should each case include a check like >>> > DRM_CLIENT_CAP_ATOMIC. >>> >>> Well all of those: >>> >>> DRM_CLIENT_CAP_STEREO_3D >>> DRM_CLIENT_CAP_UNIVERSAL_PLANES >>> DRM_CLIENT_CAP_ATOMIC >>> DRM_CLIENT_CAP_ASPECT_RATIO >>> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS >>> >>> are just usefull with KMS. >> >> It more about the semantics. If it's the first guard in the function, it >> gives the impression that the setclientcap ioctl is KMS only. If they >> are repeated for each case, then it's clear that the ioctl is more >> general and it just those caps that are KMS only. >> >> Imo, the drm_client is wider than the kms interface, but I may be wrong. Oops, slipped :-) In getcap we have 2 blocks, with DRIVER_MODESET check in between. I think a comment along the lines of /* No render-only settable capabilities for now */ /* Below caps only work with KMS drivers */ if (!drm_core_check_feature(DRIVER_MODESET)) return -ENOTSUPP; I think with that it's clear that it's _not_ the top-level check, and if someone needs to add a gem cap, they know where to put it. Not that we needed that anytime in the past 10 years, but who knows. Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 6b4a633b4240..842423fe9762 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -306,6 +306,9 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_set_client_cap *req = data; + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -ENOTSUPP; + switch (req->capability) { case DRM_CLIENT_CAP_STEREO_3D: if (req->value > 1)
All DRM_CLIENT capabilities are tied to KMS support, so returning -ENOTSUPP when KMS is not supported. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/drm_ioctl.c | 3 +++ 1 file changed, 3 insertions(+)