[PATCH] kernel/drm: vblank wait on crtc > 1

[PATCH] kernel/drm: vblank wait on crtc > 1

[Ilija Hadzic]

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__

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Ilija Hadzic]

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

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Alex Deucher]

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__
>

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Alex Deucher]

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__
>

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Ilija Hadzic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; CHARSET=X-UNKNOWN; format=flowed, Size: 4497 bytes --]



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__
>>
>

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Dave Airlie]

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.

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[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.



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.
>

OT: sending patches to the list (was: [PATCH] kernel/drm: vblank wait on crtc > 1)

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 282 bytes --]

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`

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Michel Dänzer]

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






_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Ilija Hadzic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8; format=flowed, Size: 1219 bytes --]


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
>
>
>
>
>
>
>

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Ilija Hadzic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8; format=flowed, Size: 2810 bytes --]


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.

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Alex Deucher]

On Sat, Mar 19, 2011 at 1:30 PM, Mario Kleiner
<mario.kleiner@tuebingen.mpg.de> 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.
>
>> On Fri, 18 Mar 2011, Jesse Barnes wrote:
>>>
>
>> 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.
>> --
>> Jesse Barnes, Intel Open Source Technology Center
>
> Hi
>
> A clean solution with int's in a new ioctl() would be certainly nicer. But
> if we'd define a waitvblank ioctl() v2, we should probably fix other
> limitations of the old one as well.
>
> E.g., the kernel's vblank counter is only 32 bit. The api / oml_sync_control
> extension / mesa/ x etc. expose a 64 bit vblank counter. At the moment we
> work around this by masking out the upper 32 bit in the ddx, accepting some
> small skipped frame glitch every couple of months of uptime when the "64
> bit" counter wraps around already at 32 bits. This is something we should
> probably fix in a ioctl() v2 as well.
>
> Take this just as my vote for Ilja's solution as a workable stop-gap measure
> for fixing an existing problem until we implement a more permanent solution
> for a new ioctl().

I agree.  I think this is a good stop-gap to support >2 crtcs until we
design a better ioctl.

Alex

>
> best,
> -mario
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH] kernel/drm: vblank wait on crtc > 1

[Mario Kleiner]

>
> 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.

> On Fri, 18 Mar 2011, Jesse Barnes wrote:
>>

 > 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.
 > --
 > Jesse Barnes, Intel Open Source Technology Center

Hi

A clean solution with int's in a new ioctl() would be certainly  
nicer. But if we'd define a waitvblank ioctl() v2, we should probably  
fix other limitations of the old one as well.

E.g., the kernel's vblank counter is only 32 bit. The api /  
oml_sync_control extension / mesa/ x etc. expose a 64 bit vblank  
counter. At the moment we work around this by masking out the upper  
32 bit in the ddx, accepting some small skipped frame glitch every  
couple of months of uptime when the "64 bit" counter wraps around  
already at 32 bits. This is something we should probably fix in a  
ioctl() v2 as well.

Take this just as my vote for Ilja's solution as a workable stop-gap  
measure for fixing an existing problem until we implement a more  
permanent solution for a new ioctl().

best,
-mario