[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

[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 at 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 = 0x2000, /**< Secondary display 
> > controller */
> >   _DRM_VBLANK_SIGNAL = 0x4000 /**< Send signal instead of blocking, 
> > unsupported */
> >   };
> > +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
> > +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F
> 
> 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

[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

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

[Ilija Hadzic]

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.

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

[Ilija Hadzic]

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

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

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

[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 = 0x2000, /** Secondary display 
  controller */
_DRM_VBLANK_SIGNAL = 0x4000 /** Send signal instead of blocking, 
  unsupported */
};
  +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
  +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F
 
 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]

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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul Menzel]

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`
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: 

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

[Dave Airlie]

On Sat, Mar 19, 2011 at 7:58 AM, Ilija Hadzic
 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]

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`


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Ilija Hadzic]

On Sat, 19 Mar 2011, Alex Deucher wrote:

> On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic
>  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 
>> Acked-by: Mario Kleiner 
>
> Looks good to me.
>
> Reviewed-by: Alex Deucher 
> Tested-by: Alex Deucher 
>

Signed-off-by: Ilija Hadzic 



>>
>> 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 = 0x2000, ? ? /**< Secondary display
>> controller */
>> ? ? ? ?_DRM_VBLANK_SIGNAL = 0x4000 /**< Send signal instead of blocking,
>> unsupported */
>> ?};
>> +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
>> +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F
>>
>> ?#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]

On Sat, 19 Mar 2011, Alex Deucher wrote:


On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic
ihad...@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 crtc1 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 alexdeuc...@gmail.com
Tested-by: Alex Deucher alexdeuc...@gmail.com



Signed-off-by: Ilija Hadzic ihad...@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 = 0x2000, ? ? /** Secondary display
controller */
? ? ? ?_DRM_VBLANK_SIGNAL = 0x4000 /** Send signal instead of blocking,
unsupported */
?};
+#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
+#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F

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

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

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
ihad...@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 crtc1 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.


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

[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

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

[Alex Deucher]

On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic
 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 
> Acked-by: Mario Kleiner 

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 = 0x2000, ? ? /**< Secondary display
> controller */
> ? ? ? ?_DRM_VBLANK_SIGNAL = 0x4000 /**< Send signal instead of blocking,
> unsupported */
> ?};
> +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
> +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F
>
> ?#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__
>

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

[Alex Deucher]

On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic
 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 
> Acked-by: Mario Kleiner 

Looks good to me.

Reviewed-by: Alex Deucher 
Tested-by: Alex Deucher 

>
> 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 = 0x2000, ? ? /**< Secondary display
> controller */
> ? ? ? ?_DRM_VBLANK_SIGNAL = 0x4000 /**< Send signal instead of blocking,
> unsupported */
> ?};
> +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
> +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F
>
> ?#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__
>

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

[Alex Deucher]

On Sat, Mar 19, 2011 at 1:30 PM, Mario Kleiner
 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 at 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

___
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 Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic
ihad...@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 crtc1 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 alexdeuc...@gmail.com
Tested-by: Alex Deucher alexdeuc...@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 = 0x2000,     /** Secondary display
 controller */
        _DRM_VBLANK_SIGNAL = 0x4000 /** Send signal instead of blocking,
 unsupported */
  };
 +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F

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

___
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 Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic
ihad...@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 crtc1 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 = 0x2000,     /** Secondary display
 controller */
        _DRM_VBLANK_SIGNAL = 0x4000 /** Send signal instead of blocking,
 unsupported */
  };
 +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F

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

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

[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

[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 
Acked-by: Mario Kleiner 

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 = 0x2000, /**< Secondary display 
controller */
_DRM_VBLANK_SIGNAL = 0x4000 /**< Send signal instead of blocking, 
unsupported */
  };
+#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
+#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F

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

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

[Jesse Barnes]

On Fri, 18 Mar 2011 18:13:11 -0500 (CDT)
Ilija Hadzic  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

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

[Jesse Barnes]

On Fri, 18 Mar 2011 16:58:04 -0500 (CDT)
Ilija Hadzic  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

[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 crtc1 
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 = 0x2000, /** Secondary display 
controller */
_DRM_VBLANK_SIGNAL = 0x4000 /** Send signal instead of blocking, 
unsupported */
 };
+#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16
+#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F

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

[Jesse Barnes]

On Fri, 18 Mar 2011 16:58:04 -0500 (CDT)
Ilija Hadzic ihad...@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 crtc1 
 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
___
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]

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

___
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

[Jesse Barnes]

On Fri, 18 Mar 2011 18:13:11 -0500 (CDT)
Ilija Hadzic ihad...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel