drm: Return -ENOTSUPP in drm_setclientcap() when driver do not support KMS

Submitted by José Roberto de Souza on Sept. 13, 2018, 10:13 p.m.

Details

Message ID 20180913221341.25396-1-jose.souza@intel.com
State New
Headers show
Series "drm: Return -ENOTSUPP in drm_setclientcap() when driver do not support KMS" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

José Roberto de Souza Sept. 13, 2018, 10:13 p.m.
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(+)

Patch hide | download patch | download mbox

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)

Comments

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