[1/6] drm: Let userspace check if driver supports modeset

Submitted by José Roberto de Souza on July 16, 2018, 10:38 p.m.

Details

Message ID 20180716223841.5081-1-jose.souza@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

José Roberto de Souza July 16, 2018, 10:38 p.m.
GPU accelerators usually don't have display block or the display
driver part can be disable when building driver(for servers it save
some resources) so it is important let userspace check this
capability too.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/drm_ioctl.c | 3 +++
 include/uapi/drm/drm.h      | 1 +
 2 files changed, 4 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ea10e9a26aad..3a8438ae9b51 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -244,6 +244,9 @@  static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 	case DRM_CAP_SYNCOBJ:
 		req->value = drm_core_check_feature(dev, DRIVER_SYNCOBJ);
 		return 0;
+	case DRM_CAP_MODESET:
+		req->value = drm_core_check_feature(dev, DRIVER_MODESET);
+		return 0;
 	}
 
 	/* Other caps only work with KMS drivers */
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 300f336633f2..85fae6ddbf48 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -649,6 +649,7 @@  struct drm_gem_open {
 #define DRM_CAP_PAGE_FLIP_TARGET	0x11
 #define DRM_CAP_CRTC_IN_VBLANK_EVENT	0x12
 #define DRM_CAP_SYNCOBJ		0x13
+#define DRM_CAP_MODESET		0x14
 
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {

Comments

Quoting José Roberto de Souza (2018-07-16 23:38:36)
> GPU accelerators usually don't have display block or the display
> driver part can be disable when building driver(for servers it save
> some resources) so it is important let userspace check this
> capability too.

We currently communicate that by having no modeset resources. What does
another cap bit accomplish that we don't already know?
-Chris
On Tue, 2018-07-17 at 08:28 +0100, Chris Wilson wrote:
> Quoting José Roberto de Souza (2018-07-16 23:38:36)

> > GPU accelerators usually don't have display block or the display

> > driver part can be disable when building driver(for servers it save

> > some resources) so it is important let userspace check this

> > capability too.

> 

> We currently communicate that by having no modeset resources. What

> does

> another cap bit accomplish that we don't already know?


This is a hackish way to check if driver support modeset,
drmModeGetResources()/drm_mode_getresources() can fail and return null
by other reasons and just check for the errno value can be misleading
too.

> -Chris
Quoting Souza, Jose (2018-07-17 18:02:17)
> On Tue, 2018-07-17 at 08:28 +0100, Chris Wilson wrote:
> > Quoting José Roberto de Souza (2018-07-16 23:38:36)
> > > GPU accelerators usually don't have display block or the display
> > > driver part can be disable when building driver(for servers it save
> > > some resources) so it is important let userspace check this
> > > capability too.
> > 
> > We currently communicate that by having no modeset resources. What
> > does
> > another cap bit accomplish that we don't already know?
> 
> This is a hackish way to check if driver support modeset,
> drmModeGetResources()/drm_mode_getresources() can fail and return null
> by other reasons and just check for the errno value can be misleading
> too.

Do not confuse libdrm with the ioctl. You do not need to allocate
anything for such a check, and it is being used directly without
allocations as a has-kms check.

More to the point existing userspace determines modeset capability
through the reported resources. Your changelog needs to explain why they
are inadequate and how you plan to coordinate your fix with userspace.
As it stands you are introducing uABI with no user...
-Chris
On Tue, Jul 17, 2018 at 09:04:45PM +0100, Chris Wilson wrote:
> Quoting Souza, Jose (2018-07-17 18:02:17)
> > On Tue, 2018-07-17 at 08:28 +0100, Chris Wilson wrote:
> > > Quoting José Roberto de Souza (2018-07-16 23:38:36)
> > > > GPU accelerators usually don't have display block or the display
> > > > driver part can be disable when building driver(for servers it save
> > > > some resources) so it is important let userspace check this
> > > > capability too.
> > > 
> > > We currently communicate that by having no modeset resources. What
> > > does
> > > another cap bit accomplish that we don't already know?
> > 
> > This is a hackish way to check if driver support modeset,
> > drmModeGetResources()/drm_mode_getresources() can fail and return null
> > by other reasons and just check for the errno value can be misleading
> > too.
> 
> Do not confuse libdrm with the ioctl. You do not need to allocate
> anything for such a check, and it is being used directly without
> allocations as a has-kms check.
> 
> More to the point existing userspace determines modeset capability
> through the reported resources. Your changelog needs to explain why they
> are inadequate and how you plan to coordinate your fix with userspace.
> As it stands you are introducing uABI with no user...

I agree with Chris here. There is no indication on how this will
affect userspace here and this so far is just a uABI without user
that is a big blocker.

And I also got confused because that modeset capability check
got introduced in a block that is for
"/* Only some caps make sense with UMS/render-only drivers. */"

> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2018-08-15 at 14:15 -0700, Rodrigo Vivi wrote:
> On Tue, Jul 17, 2018 at 09:04:45PM +0100, Chris Wilson wrote:

> > Quoting Souza, Jose (2018-07-17 18:02:17)

> > > On Tue, 2018-07-17 at 08:28 +0100, Chris Wilson wrote:

> > > > Quoting José Roberto de Souza (2018-07-16 23:38:36)

> > > > > GPU accelerators usually don't have display block or the

> > > > > display

> > > > > driver part can be disable when building driver(for servers

> > > > > it save

> > > > > some resources) so it is important let userspace check this

> > > > > capability too.

> > > > 

> > > > We currently communicate that by having no modeset resources.

> > > > What

> > > > does

> > > > another cap bit accomplish that we don't already know?

> > > 

> > > This is a hackish way to check if driver support modeset,

> > > drmModeGetResources()/drm_mode_getresources() can fail and return

> > > null

> > > by other reasons and just check for the errno value can be

> > > misleading

> > > too.

> > 

> > Do not confuse libdrm with the ioctl. You do not need to allocate

> > anything for such a check, and it is being used directly without

> > allocations as a has-kms check.

> > 

> > More to the point existing userspace determines modeset capability

> > through the reported resources. Your changelog needs to explain why

> > they

> > are inadequate and how you plan to coordinate your fix with

> > userspace.

> > As it stands you are introducing uABI with no user...

> 

> I agree with Chris here. There is no indication on how this will

> affect userspace here and this so far is just a uABI without user

> that is a big blocker.


The user would be IGT but I would only send the patch after get this
merged.

Okay, I can drop this patch without any changes in this series.

> 

> And I also got confused because that modeset capability check

> got introduced in a block that is for

> "/* Only some caps make sense with UMS/render-only drivers. */"

> 

> > -Chris

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx