Hi Dave,
Below is a patch against drm-next branch of 2.6.38-rc8+ kernel that adds the capability to wait on vblank events for CRTCs that are greater than 1 and thus cannot be represented with primary/secondary flags in the legacy interface. It was discussed on the dri-devel list in these two threads:
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
Regards,
Ilija
Reviewed-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de> Acked-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 7f6912a..3617b4c 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -280,6 +280,9 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv) if (dev->driver->dumb_create) req->value = 1; break; + case DRM_CAP_HIGH_CRTC: + req->value = 1; + break; default: return -EINVAL; } diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a34ef97..c725088 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1125,7 +1125,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, { union drm_wait_vblank *vblwait = data; int ret = 0; - unsigned int flags, seq, crtc; + unsigned int flags, seq, crtc, high_crtc;
if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) return -EINVAL; @@ -1134,16 +1134,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data, return -EINVAL;
if (vblwait->request.type & - ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)) { + ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | + _DRM_VBLANK_HIGH_CRTC_MASK)) { DRM_ERROR("Unsupported type value 0x%x, supported mask 0x%x\n", vblwait->request.type, - (_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)); + (_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | + _DRM_VBLANK_HIGH_CRTC_MASK)); return -EINVAL; }
flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK; - crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0; - + high_crtc = (vblwait->request.type & _DRM_VBLANK_HIGH_CRTC_MASK); + if (high_crtc) + crtc = high_crtc >> _DRM_VBLANK_HIGH_CRTC_SHIFT; + else + crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0; if (crtc >= dev->num_crtcs) return -EINVAL;
diff --git a/include/drm/drm.h b/include/drm/drm.h index 9ac4313..99cd074 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -469,6 +469,8 @@ enum drm_vblank_seq_type { _DRM_VBLANK_SECONDARY = 0x20000000, /**< Secondary display controller */ _DRM_VBLANK_SIGNAL = 0x40000000 /**< Send signal instead of blocking, unsupported */ }; +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F0000
#define _DRM_VBLANK_TYPES_MASK (_DRM_VBLANK_ABSOLUTE | _DRM_VBLANK_RELATIVE) #define _DRM_VBLANK_FLAGS_MASK (_DRM_VBLANK_EVENT | _DRM_VBLANK_SIGNAL | \ @@ -753,6 +755,7 @@ struct drm_event_vblank { };
#define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2
/* typedef area */ #ifndef __KERNEL__
On Fri, 18 Mar 2011 16:58:04 -0500 (CDT) Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Dave,
Below is a patch against drm-next branch of 2.6.38-rc8+ kernel that adds the capability to wait on vblank events for CRTCs that are greater than 1 and thus cannot be represented with primary/secondary flags in the legacy interface. It was discussed on the dri-devel list in these two threads:
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
I like the new param check, but I'd still prefer a new ioctl to abusing the old one.
On Fri, 18 Mar 2011, Jesse Barnes wrote:
I like the new param check, but I'd still prefer a new ioctl to abusing the old one.
It's not "abusing" it but extending the interface in a backwards-compatible manner. Introducing a new one would result in two ioctls that essentially do the same thing, which I don't like.
-- Ilija
On Fri, 18 Mar 2011 18:13:11 -0500 (CDT) Ilija Hadzic ihadzic@research.bell-labs.com wrote:
On Fri, 18 Mar 2011, Jesse Barnes wrote:
I like the new param check, but I'd still prefer a new ioctl to abusing the old one.
It's not "abusing" it but extending the interface in a backwards-compatible manner. Introducing a new one would result in two ioctls that essentially do the same thing, which I don't like.
Yes abusing was a strong word; I just don't like encoding crtc numbers in a bitfield, when we just use ints everywhere else.
Not a big deal, Dave will make the final call. Thanks for doing this work.
On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Dave,
Below is a patch against drm-next branch of 2.6.38-rc8+ kernel that adds the capability to wait on vblank events for CRTCs that are greater than 1 and thus cannot be represented with primary/secondary flags in the legacy interface. It was discussed on the dri-devel list in these two threads:
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
Regards,
Ilija
Reviewed-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de> Acked-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
Looks good to me.
Reviewed-by: Alex Deucher alexdeucher@gmail.com Tested-by: Alex Deucher alexdeucher@gmail.com
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 7f6912a..3617b4c 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -280,6 +280,9 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv) if (dev->driver->dumb_create) req->value = 1; break;
- case DRM_CAP_HIGH_CRTC:
- req->value = 1;
- break;
default: return -EINVAL; } diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a34ef97..c725088 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1125,7 +1125,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, { union drm_wait_vblank *vblwait = data; int ret = 0;
- unsigned int flags, seq, crtc;
- unsigned int flags, seq, crtc, high_crtc;
if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) return -EINVAL; @@ -1134,16 +1134,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data, return -EINVAL;
if (vblwait->request.type &
- ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)) {
- ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | +
_DRM_VBLANK_HIGH_CRTC_MASK)) { DRM_ERROR("Unsupported type value 0x%x, supported mask 0x%x\n", vblwait->request.type,
- (_DRM_VBLANK_TYPES_MASK |
_DRM_VBLANK_FLAGS_MASK));
- (_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |
- _DRM_VBLANK_HIGH_CRTC_MASK));
return -EINVAL; }
flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK;
- crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
- high_crtc = (vblwait->request.type & _DRM_VBLANK_HIGH_CRTC_MASK);
- if (high_crtc)
- crtc = high_crtc >> _DRM_VBLANK_HIGH_CRTC_SHIFT;
- else
- crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
if (crtc >= dev->num_crtcs) return -EINVAL;
diff --git a/include/drm/drm.h b/include/drm/drm.h index 9ac4313..99cd074 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -469,6 +469,8 @@ enum drm_vblank_seq_type { _DRM_VBLANK_SECONDARY = 0x20000000, /**< Secondary display controller */ _DRM_VBLANK_SIGNAL = 0x40000000 /**< Send signal instead of blocking, unsupported */ }; +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F0000
#define _DRM_VBLANK_TYPES_MASK (_DRM_VBLANK_ABSOLUTE | _DRM_VBLANK_RELATIVE) #define _DRM_VBLANK_FLAGS_MASK (_DRM_VBLANK_EVENT | _DRM_VBLANK_SIGNAL | \ @@ -753,6 +755,7 @@ struct drm_event_vblank { };
#define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2
/* typedef area */ #ifndef __KERNEL__
On Sat, 19 Mar 2011, Alex Deucher wrote:
On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Dave,
Below is a patch against drm-next branch of 2.6.38-rc8+ kernel that adds the capability to wait on vblank events for CRTCs that are greater than 1 and thus cannot be represented with primary/secondary flags in the legacy interface. It was discussed on the dri-devel list in these two threads:
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
Regards,
Ilija
Reviewed-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de> Acked-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
Looks good to me.
Reviewed-by: Alex Deucher alexdeucher@gmail.com Tested-by: Alex Deucher alexdeucher@gmail.com
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 7f6912a..3617b4c 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -280,6 +280,9 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv) � � � � � � � �if (dev->driver->dumb_create) � � � � � � � � � � � �req->value = 1; � � � � � � � �break;
- � � � case DRM_CAP_HIGH_CRTC:
- � � � � � � � req->value = 1;
- � � � � � � � break;
� � � �default: � � � � � � � �return -EINVAL; � � � �} diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a34ef97..c725088 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1125,7 +1125,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, �{ � � � �union drm_wait_vblank *vblwait = data; � � � �int ret = 0;
- � � � unsigned int flags, seq, crtc;
- � � � unsigned int flags, seq, crtc, high_crtc;
� � � �if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) � � � � � � � �return -EINVAL; @@ -1134,16 +1134,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data, � � � � � � � �return -EINVAL;
� � � �if (vblwait->request.type &
- � � � � � ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)) {
- � � � � � ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | +
� _DRM_VBLANK_HIGH_CRTC_MASK)) { � � � � � � � �DRM_ERROR("Unsupported type value 0x%x, supported mask 0x%x\n", � � � � � � � � � � � � �vblwait->request.type,
- � � � � � � � � � � � � (_DRM_VBLANK_TYPES_MASK |
_DRM_VBLANK_FLAGS_MASK));
- � � � � � � � � � � � � (_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |
- � � � � � � � � � � _DRM_VBLANK_HIGH_CRTC_MASK));
� � � � � � � �return -EINVAL; � � � �}
� � � �flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK;
- � � � crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
- � � � high_crtc = (vblwait->request.type & _DRM_VBLANK_HIGH_CRTC_MASK);
- � � � if (high_crtc)
- � � � � � � � crtc = high_crtc >> _DRM_VBLANK_HIGH_CRTC_SHIFT;
- � � � else
- � � � � � � � crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
� � � �if (crtc >= dev->num_crtcs) � � � � � � � �return -EINVAL;
diff --git a/include/drm/drm.h b/include/drm/drm.h index 9ac4313..99cd074 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -469,6 +469,8 @@ enum drm_vblank_seq_type { � � � �_DRM_VBLANK_SECONDARY = 0x20000000, � � /**< Secondary display controller */ � � � �_DRM_VBLANK_SIGNAL = 0x40000000 /**< Send signal instead of blocking, unsupported */ �}; +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F0000
�#define _DRM_VBLANK_TYPES_MASK (_DRM_VBLANK_ABSOLUTE | _DRM_VBLANK_RELATIVE) �#define _DRM_VBLANK_FLAGS_MASK (_DRM_VBLANK_EVENT | _DRM_VBLANK_SIGNAL | \ @@ -753,6 +755,7 @@ struct drm_event_vblank { �};
�#define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2
�/* typedef area */ �#ifndef __KERNEL__
On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Dave,
Below is a patch against drm-next branch of 2.6.38-rc8+ kernel that adds the capability to wait on vblank events for CRTCs that are greater than 1 and thus cannot be represented with primary/secondary flags in the legacy interface. It was discussed on the dri-devel list in these two threads:
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
Regards,
Ilija
Reviewed-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de> Acked-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
Ilija, please add your signed-off-by.
Alex
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 7f6912a..3617b4c 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -280,6 +280,9 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv) if (dev->driver->dumb_create) req->value = 1; break;
- case DRM_CAP_HIGH_CRTC:
- req->value = 1;
- break;
default: return -EINVAL; } diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a34ef97..c725088 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1125,7 +1125,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, { union drm_wait_vblank *vblwait = data; int ret = 0;
- unsigned int flags, seq, crtc;
- unsigned int flags, seq, crtc, high_crtc;
if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) return -EINVAL; @@ -1134,16 +1134,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data, return -EINVAL;
if (vblwait->request.type &
- ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)) {
- ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | +
_DRM_VBLANK_HIGH_CRTC_MASK)) { DRM_ERROR("Unsupported type value 0x%x, supported mask 0x%x\n", vblwait->request.type,
- (_DRM_VBLANK_TYPES_MASK |
_DRM_VBLANK_FLAGS_MASK));
- (_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |
- _DRM_VBLANK_HIGH_CRTC_MASK));
return -EINVAL; }
flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK;
- crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
- high_crtc = (vblwait->request.type & _DRM_VBLANK_HIGH_CRTC_MASK);
- if (high_crtc)
- crtc = high_crtc >> _DRM_VBLANK_HIGH_CRTC_SHIFT;
- else
- crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
if (crtc >= dev->num_crtcs) return -EINVAL;
diff --git a/include/drm/drm.h b/include/drm/drm.h index 9ac4313..99cd074 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -469,6 +469,8 @@ enum drm_vblank_seq_type { _DRM_VBLANK_SECONDARY = 0x20000000, /**< Secondary display controller */ _DRM_VBLANK_SIGNAL = 0x40000000 /**< Send signal instead of blocking, unsupported */ }; +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F0000
#define _DRM_VBLANK_TYPES_MASK (_DRM_VBLANK_ABSOLUTE | _DRM_VBLANK_RELATIVE) #define _DRM_VBLANK_FLAGS_MASK (_DRM_VBLANK_EVENT | _DRM_VBLANK_SIGNAL | \ @@ -753,6 +755,7 @@ struct drm_event_vblank { };
#define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2
/* typedef area */ #ifndef __KERNEL__
On Sat, Mar 19, 2011 at 7:58 AM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Dave,
Below is a patch against drm-next branch of 2.6.38-rc8+ kernel that adds the capability to wait on vblank events for CRTCs that are greater than 1 and thus cannot be represented with primary/secondary flags in the legacy interface. It was discussed on the dri-devel list in these two threads:
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
Yeah I'm happy with this, however your mail reader seems to have eaten the patch.
I've rescued it locally, but next time please make sure the patch hasn't been whitespace damaged on the way out.
lots of spaces before tabs added.
Dave.
sorry about that, I use pine and thought that's as plain as it gets. I guess next time I'll try just 'mail' from command line.
On Mon, 21 Mar 2011, Dave Airlie wrote:
On Sat, Mar 19, 2011 at 7:58 AM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Dave,
Below is a patch against drm-next branch of 2.6.38-rc8+ kernel that adds the capability to wait on vblank events for CRTCs that are greater than 1 and thus cannot be represented with primary/secondary flags in the legacy interface. It was discussed on the dri-devel list in these two threads:
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
Yeah I'm happy with this, however your mail reader seems to have eaten the patch.
I've rescued it locally, but next time please make sure the patch hasn't been whitespace damaged on the way out.
lots of spaces before tabs added.
Dave.
Am Sonntag, den 20.03.2011, 18:47 -0500 schrieb Ilija Hadzic:
sorry about that, I use pine and thought that's as plain as it gets. I guess next time I'll try just 'mail' from command line.
Or `git send-email`¹.
Thanks,
Paul
¹ manual: `git help send-email`
On Fre, 2011-03-18 at 16:58 -0500, Ilija Hadzic wrote:
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
[...]
You seem to have silently ignored my previous concerns and suggestions about the handling of the high CRTC mask/shift.
@@ -753,6 +755,7 @@ struct drm_event_vblank { };
#define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2
Seems like a rather generic name, something like DRM_CAP_VBLANK_HIGH_CRTC might be better.
Unless I oversaw something nothing was silently ignored. I believe I responded to each of your comments (and comments by others), those I agreed with I implemented, those I didn't agree with I didn't implement.
-- Ilija
On Tue, 22 Mar 2011, Michel [ISO-8859-1] D�nzer wrote:
On Fre, 2011-03-18 at 16:58 -0500, Ilija Hadzic wrote:
This patch extends the interface to drm_wait_vblank ioctl so that crtc>1 can be represented. It also adds a new capability to drm_getcap ioctl so that the user space can check whether the new interface to drm_wait_vblank is supported (and fall back to the legacy interface if not)
[...]
You seem to have silently ignored my previous concerns and suggestions about the handling of the high CRTC mask/shift.
@@ -753,6 +755,7 @@ struct drm_event_vblank { };
#define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2
Seems like a rather generic name, something like DRM_CAP_VBLANK_HIGH_CRTC might be better.
-- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer
On Die, 2011-03-22 at 06:16 -0500, Ilija Hadzic wrote:
Unless I oversaw something nothing was silently ignored. I believe I responded to each of your comments (and comments by others), those I agreed with I implemented, those I didn't agree with I didn't implement.
I haven't seen any response to the below excerpts from 1299251679.14068.83.camel@thor.local and the post you replied to:
------------------------- kernel patch ----------------------------------------
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 16d5155..3b0abae 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -677,16 +677,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data, return -EINVAL;
if (vblwait->request.type &
~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)) {
~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |_DRM_VBLANK_HIGH_CRTC_MASK)) { DRM_ERROR("Unsupported type value 0x%x, supported mask 0x%x\n", vblwait->request.type,
(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK));
(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |_DRM_VBLANK_HIGH_CRTC_MASK)); return -EINVAL; }If _DRM_VBLANK_HIGH_CRTC_MASK were included in _DRM_VBLANK_FLAGS_MASK (or _DRM_VBLANK_TYPES_MASK, but that would make less sense), these changes shouldn't be necessary.
diff --git a/include/drm/drm.h b/include/drm/drm.h index e5f7061..d950581 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -469,6 +469,8 @@ enum drm_vblank_seq_type { _DRM_VBLANK_SECONDARY = 0x20000000, /**< Secondary display controller */ _DRM_VBLANK_SIGNAL = 0x40000000 /**< Send signal instead of blocking, unsupported */ }; +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F0000
I'd suggest making _DRM_VBLANK_HIGH_CRTC_SHIFT either 21 (so _DRM_VBLANK_HIGH_CRTC_MASK is adjacent to _DRM_VBLANK_EVENT) or much lower, say 8 or even 4, as the flags are more likely to get extended than the types, if history is any indication.
Also,
#define _DRM_VBLANK_HIGH_CRTC_MASK (0x1F << _DRM_VBLANK_HIGH_CRTC_SHIFT)
would make it obvious how these values are related and decrease the likelihood of divergence during development of the patch.
[...]
@@ -753,6 +755,7 @@ struct drm_event_vblank { };
#define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2
Seems like a rather generic name, something like DRM_CAP_VBLANK_HIGH_CRTC might be better.
Sorry about overseeing additional comments. It definitely wasn't intentional.
On Tue, 22 Mar 2011, Michel [ISO-8859-1] D�nzer wrote:
If _DRM_VBLANK_HIGH_CRTC_MASK were included in _DRM_VBLANK_FLAGS_MASK (or _DRM_VBLANK_TYPES_MASK, but that would make less sense), these changes shouldn't be necessary.
HIGH_CRTC does not logically belong either flags nor types, but it's a third thing and hence the separate mask. If I stashed it under either, then this would really be an "abuse" (as Jesse nicely put it) of an existing ioctl. This way it is just adding a new bit section/mask to a filed that already has multiple of them.
I'd suggest making _DRM_VBLANK_HIGH_CRTC_SHIFT either 21 (so _DRM_VBLANK_HIGH_CRTC_MASK is adjacent to _DRM_VBLANK_EVENT) or much lower, say 8 or even 4, as the flags are more likely to get extended than the types, if history is any indication.
I can't have an opinion on that because I wasn't around (at least not in the graphics subsystem world) to see the historic development of this ioctl. However, I'd say that the motivation is rather speculative.
However, from pragmatic angle, at this point the change is already in (it was in drm-core-next, last time in checked) and it is probably not a good idea to shift things around and thus potentially create multiple incompatible combinations of user spaces and kernels (even if they are just test builds). Especially that the suggestion is based more on what *may* happen in the future evolution of this ioctl rather than some funamental problem. Furthermore, we have heard on the list that at the end of the day, the "evolution" of this ioctl will be the basis for (later) redesigning the new one and getting it better in many other aspects. If that's indeed so, that's even less of a motivation to split hair.
Unless I hear strong voice from others on the list to change the position of HIGH_CRTC mask, I am reluctant to touch it at this point.
Also,
#define _DRM_VBLANK_HIGH_CRTC_MASK (0x1F << _DRM_VBLANK_HIGH_CRTC_SHIFT)
would make it obvious how these values are related and decrease the likelihood of divergence during development of the patch.
I agree, it's a "cosmetic" change, though, but it indeed improves the code readibility/maintainability, so I will submit a follow-up patch (again, when I get some spare time to attend to this, given my day-job engagements).
[...]
@@ -753,6 +755,7 @@ struct drm_event_vblank { };
#define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2
Seems like a rather generic name, something like DRM_CAP_VBLANK_HIGH_CRTC might be better.
will be in the follow-up patch mentioned above.
On Die, 2011-03-22 at 08:43 -0500, Ilija Hadzic wrote:
On Tue, 22 Mar 2011, Michel [ISO-8859-1] Dnzer wrote:
If _DRM_VBLANK_HIGH_CRTC_MASK were included in _DRM_VBLANK_FLAGS_MASK (or _DRM_VBLANK_TYPES_MASK, but that would make less sense), these changes shouldn't be necessary.
HIGH_CRTC does not logically belong either flags nor types, but it's a third thing and hence the separate mask.
I'd say 'logically' it belongs with _DRM_VBLANK_SECONDARY, i.e. _DRM_VBLANK_FLAGS_MASK .
I'd suggest making _DRM_VBLANK_HIGH_CRTC_SHIFT either 21 (so _DRM_VBLANK_HIGH_CRTC_MASK is adjacent to _DRM_VBLANK_EVENT) or much lower, say 8 or even 4, as the flags are more likely to get extended than the types, if history is any indication.
I can't have an opinion on that because I wasn't around (at least not in the graphics subsystem world) to see the historic development of this ioctl. However, I'd say that the motivation is rather speculative.
However, from pragmatic angle, at this point the change is already in (it was in drm-core-next, last time in checked) and it is probably not a good idea to shift things around and thus potentially create multiple incompatible combinations of user spaces and kernels (even if they are just test builds).
It's indeed unfortunate that the patch was applied despite outstanding issues, but I don't think this is really a problem before it hits mainline, and the userspace bits haven't been applied anywhere yet.
Especially that the suggestion is based more on what *may* happen in the future evolution of this ioctl rather than some funamental problem. Furthermore, we have heard on the list that at the end of the day, the "evolution" of this ioctl will be the basis for (later) redesigning the new one and getting it better in many other aspects. If that's indeed so, that's even less of a motivation to split hair.
I don't think it makes sense to replace the ioctl with a new one until there's something that cannot (reasonably) be done with the existing one. Migrating userspace to the new ioctl would incur pain of its own, and more likely than not the new ioctl would at some point later turn out to have its own issues and limitations as well.
Unless I hear strong voice from others on the list to change the position of HIGH_CRTC mask, I am reluctant to touch it at this point.
You failed to take simple suggestions to make the patch smaller and more future-proof at once in several weeks, I'm afraid 'at this point' isn't a very good argument in light of that.
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Dave Airlie -
Ilija Hadzic -
Jesse Barnes -
Michel Dänzer -
Paul Menzel