vblank problem (and proposed fix) on crtc > 1

[Michel Dänzer]

On Don, 2011-03-10 at 07:32 -0600, Ilija Hadzic wrote: 
> 
> On Thu, 10 Mar 2011, Michel [ISO-8859-1] Dnzer wrote:
> 
> > I'm afraid I don't really like this. Apart from the ugly bogus error
> > message, the probes could fail for other reasons, e.g. at some point we
> > might want to return an error when the ioctl is called for a CRTC which
> > is currently disabled, to avoid the hang you were getting for CRTC 1.
> 
> Your example actually speaks in favor of probing. Let's say that the 
> future kernel starts returning an error for disabled CRTC and you don't do 
> any probing. DDX will start calling vblank waits on disabled CRTC (just 
> like it's doing now) and spam the kernel with errors.

No, userspace shouldn't call the ioctl for a disabled CRTC during normal
operation, that would be a bug of its own. However, a CRTC could be
disabled during the probe but get enabled later, e.g. due to
hot-plugging a display.


> > Dave's drm-next tree actually has a new DRM_IOCTL_GET_CAP ioctl, which
> > could be used for detecting this capability. It might even be possible
> > to handle old kernels transparently in drmWaitVBlank(), but I'm not sure
> > offhand if that would be better overall than doing it in the driver
> > code.
> 
> This argument is self defeating. The purpose of the probing is to check 
> for the old kernel. If I have DRM_IOCTL_GET_CAP, that means that I have a 
> new kernel so probing will just work. If I have an old kernel then I don't 
> have DRM_IOCTL_GET_CAP ioctl, so attemting to use it will result in an 
> error from the kernel anyway. It makes no sense to me to cause an error 
> in one ioctl so that you would prevent an error in other ioctl.

The difference is that there should be no bogus error message in dmesg,
avoiding spurious bug reports.


> >> +   if (info->high_crtc_works) {
> >> +   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
> >> +   DRM_VBLANK_HIGH_CRTC_MASK;
> >> +   } else vbl.request.type |= DRM_VBLANK_SECONDARY;
> >
> > Waiting on a random other CRTC makes no sense. If you know you can't
> > wait on the given CRTC, don't wait at all.
> >
> 
> It's not random CRTC, but it's what today's DDX and DRM call "secondary" 
> CRTC. I intentionally did this for two reasons. First, this is the 
> behavior that is identical to what we see today, and I prefer to preserve 
> the same behavior if either component is old (DDX or kernel), rather than 
> have different behavior for different combinations of old/new.

I don't see the value in conserving fundamentally broken behaviour.

> The second reason is pragmatic, if you want to not call wait_vblank at
> all and make sure you are safe to whatever the code above you is doing
> and not return an error you have to make up the sequence numbers and
> timestamps (and probably keep track of them) and that's much more
> radical modification to the DDX.

You could probably always return 0, or previous value + 1 or whatever,
that's no more wrong than the values from a different CRTC.


> >> +   vbl.request.sequence = 0;
> >> +   if (drmWaitVBlank(info->dri2.drm_fd, )) {
> >> +   xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Kernel rejects 
> >> VBLANK request on CRTC %d\n", c);
> >> +   info->high_crtc_works = FALSE;
> >
> > Should break out of the for loop at this point.
> 
> This was also intentional. I want to see in Xorg log file the full list of 
> crtcs that rejected the vblank wait request. It comes handy for analyzing 
> the problems and/or bugs.

Then at the very least the claim that 'an error message will appear in
the kernel only once at start up time' is wrong, there will actually be
four of them on old kernels.


-- 
Earthling Michel D?nzer   |http://www.vmware.com
Libre software enthusiast |  Debian, X and DRI developer

vblank problem (and proposed fix) on crtc > 1

[Michel Dänzer]

On Mit, 2011-03-09 at 11:33 -0600, Ilija Hadzic wrote: 
> 
> So what I did is to actually probe the kernel at screen init time. 
> When the screen is initialized, the DDX checks if the GPU has more than
> two CRTCs and tries a dummy vblank wait on all crtcs > 1. If any
> of them fails, it falls back to the old method of using only
> primary/secondary flag. 

I'm afraid I don't really like this. Apart from the ugly bogus error
message, the probes could fail for other reasons, e.g. at some point we
might want to return an error when the ioctl is called for a CRTC which
is currently disabled, to avoid the hang you were getting for CRTC 1.


Dave's drm-next tree actually has a new DRM_IOCTL_GET_CAP ioctl, which
could be used for detecting this capability. It might even be possible
to handle old kernels transparently in drmWaitVBlank(), but I'm not sure
offhand if that would be better overall than doing it in the driver
code.


Some detailed technical comments:

> diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> index 66df03c..c45abe6 100644
> --- a/src/radeon_dri2.c
> +++ b/src/radeon_dri2.c
> @@ -1145,8 +1193,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
> DrawablePtr draw,
>   vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
>   if (flip == 0)
>   vbl.request.type |= DRM_VBLANK_NEXTONMISS;
> -if (crtc > 0)
> +if (crtc == 1)
>   vbl.request.type |= DRM_VBLANK_SECONDARY;
> +else if (crtc > 1) {
> +   if (info->high_crtc_works) {
> +   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & 
> +   DRM_VBLANK_HIGH_CRTC_MASK;
> +   } else vbl.request.type |= DRM_VBLANK_SECONDARY;

Waiting on a random other CRTC makes no sense. If you know you can't
wait on the given CRTC, don't wait at all. 


> @@ -1261,6 +1319,22 @@ radeon_dri2_screen_init(ScreenPtr pScreen)
>   xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "You need a newer kernel 
> for sync extension\n");
>   }
> 
> +if (info->drmmode.mode_res->count_crtcs > 2) {
> +   xf86DrvMsg(pScrn->scrnIndex, X_INFO, "GPU with %d CRTCs found, 
> probing VBLANKs on CRTC>1\n",
> +  info->drmmode.mode_res->count_crtcs); 
> +   info->high_crtc_works = TRUE;
> +   for (c=2; cdrmmode.mode_res->count_crtcs; c++) {
> +   vbl.request.type = DRM_VBLANK_RELATIVE;
> +   vbl.request.type |= (c << DRM_VBLANK_HIGH_CRTC_SHIFT) & 
> DRM_VBLANK_HIGH_CRTC_MASK;
> +   vbl.request.sequence = 0;
> +   if (drmWaitVBlank(info->dri2.drm_fd, )) {
> +   xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Kernel rejects 
> VBLANK request on CRTC %d\n", c);
> +   info->high_crtc_works = FALSE;

Should break out of the for loop at this point. 

> +   }
> +   }
> +   if (info->high_crtc_works) xf86DrvMsg(pScrn->scrnIndex, X_INFO, 
> "VBLANK request accepted on all CRTCs\n"); 

The statement guarded by the if condition needs to go on a new line, and
please don't add trailing whitespace. Also, again, please match the
indentation of the surrounding code.


-- 
Earthling Michel D?nzer   |http://www.vmware.com
Libre software enthusiast |  Debian, X and DRI developer

vblank problem (and proposed fix) on crtc > 1

[Ilija Hadzic]

On Thu, 10 Mar 2011, Michel [ISO-8859-1] D?nzer wrote:

> No, userspace shouldn't call the ioctl for a disabled CRTC during normal
> operation, that would be a bug of its own.

Well, that's exactly what DDX is doing today. Try this experiment: open a 
terminal window and run glxgears in it and observer the frame rate synced 
up to your monitor. Then in another terminal window type 'xset dpms force 
off' (or go to dpms off state any other way you want). The screen goes 
blank (disabled CRTC). Wait a while and then move your mouse to exit the 
disabled state. Notice the low frame rate reported by glxgears in one 
line. That's because glxSwapBuffers was blocked while screen was in 
power-down state.

Now I don't know if this behavior is intentional (I know your opinion 
based on the above), but it looks logical to me: if the screen is off and 
GLX attempts to wait on it, it gets what it deserves. If this is the real 
intent, then kernel returning error on disabled screen would break it (the 
application would just start to "rush" through rendering at max speed and 
eat up the I/O and CPU bandwidth for something that you can't see on the 
screen). Anyway, this is a topic for a separate thread.

I tend to agree that one day maybe the error is returned for some other 
reason (where your example may have been a bad one) so I will look into 
DRM_IOCTL_GET_CAP.

>
> The difference is that there should be no bogus error message in dmesg,
> avoiding spurious bug reports.
>

That I agree and if GET_CAP ioctl is appropriate here, I'll change to 
that. There will still be an error message in the kernel at probing time, 
but hopefully a more meaningfull one. I guess the Xorg log message should 
then be something along the "you need a newer kernel" lines. Agreed ?

>> the same behavior if either component is old (DDX or kernel), rather than
>> have different behavior for different combinations of old/new.
>
> I don't see the value in conserving fundamentally broken behaviour.
>

There is even less value in introducing a wider variety of broken 
behaviors. Returning immediately, would cause the application to rush at 
full speed through buffer swaps and eat up the CPU and I/O bandwidth.
It think that there are equal number of ppl. who would argue for that as 
those who would argue for the opposite.

>
> You could probably always return 0, or previous value + 1 or whatever,
> that's no more wrong than the values from a different CRTC.
>

What I could do doesn't matter that much, because whatever I do would not 
result it fully functional system unless both the DDX and the kernel match 
in capabilities. So I go for the one that is less likely to introduce new 
bugs or unpredictable behavior.

>
> Then at the very least the claim that 'an error message will appear in
> the kernel only once at start up time' is wrong, there will actually be
> four of them on old kernels.
>

More precisely: num_crtcs - 2 times. Use word 'once' used in a liberal 
sense ;-) it doesn't happen repetatively during the normal operation.
At least, if I can use DRM_IOCTL_GET_CAP (which I still have to check),
the message will be more meaningful.

vblank problem (and proposed fix) on crtc > 1

[Ilija Hadzic]

On Thu, 10 Mar 2011, Michel [ISO-8859-1] D?nzer wrote:

>
> I'm afraid I don't really like this. Apart from the ugly bogus error
> message, the probes could fail for other reasons, e.g. at some point we
> might want to return an error when the ioctl is called for a CRTC which
> is currently disabled, to avoid the hang you were getting for CRTC 1.
>

Your example actually speaks in favor of probing. Let's say that the 
future kernel starts returning an error for disabled CRTC and you don't do 
any probing. DDX will start calling vblank waits on disabled CRTC (just 
like it's doing now) and spam the kernel with errors. With probing, error 
will happen only once at screen init time.

> Dave's drm-next tree actually has a new DRM_IOCTL_GET_CAP ioctl, which
> could be used for detecting this capability. It might even be possible
> to handle old kernels transparently in drmWaitVBlank(), but I'm not sure
> offhand if that would be better overall than doing it in the driver
> code.
>

This argument is self defeating. The purpose of the probing is to check 
for the old kernel. If I have DRM_IOCTL_GET_CAP, that means that I have a 
new kernel so probing will just work. If I have an old kernel then I don't 
have DRM_IOCTL_GET_CAP ioctl, so attemting to use it will result in an 
error from the kernel anyway. It makes no sense to me to cause an error 
in one ioctl so that you would prevent an error in other ioctl.

>> +   if (info->high_crtc_works) {
>> +   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
>> +   DRM_VBLANK_HIGH_CRTC_MASK;
>> +   } else vbl.request.type |= DRM_VBLANK_SECONDARY;
>
> Waiting on a random other CRTC makes no sense. If you know you can't
> wait on the given CRTC, don't wait at all.
>

It's not random CRTC, but it's what today's DDX and DRM call "secondary" 
CRTC. I intentionally did this for two reasons. First, this is the 
behavior that is identical to what we see today, and I prefer to preserve 
the same behavior if either component is old (DDX or kernel), rather than 
have different behavior for different combinations of old/new. The second 
reason is pragmatic, if you want to not call wait_vblank at all and make 
sure you are safe to whatever the code above you is doing and not 
return an error you have to make up the sequence numbers and timestamps
(and probably keep track of them) and that's much more radical 
modification to the DDX. So I figured this would be conservative.

>> +   vbl.request.sequence = 0;
>> +   if (drmWaitVBlank(info->dri2.drm_fd, )) {
>> +   xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Kernel rejects 
>> VBLANK request on CRTC %d\n", c);
>> +   info->high_crtc_works = FALSE;
>
> Should break out of the for loop at this point.
>

This was also intentional. I want to see in Xorg log file the full list of 
crtcs that rejected the vblank wait request. It comes handy for analyzing 
the problems and/or bugs.

Re: vblank problem (and proposed fix) on crtc 1

[Michel Dänzer]

On Mit, 2011-03-09 at 11:33 -0600, Ilija Hadzic wrote: 
 
 So what I did is to actually probe the kernel at screen init time. 
 When the screen is initialized, the DDX checks if the GPU has more than
 two CRTCs and tries a dummy vblank wait on all crtcs  1. If any
 of them fails, it falls back to the old method of using only
 primary/secondary flag. 

I'm afraid I don't really like this. Apart from the ugly bogus error
message, the probes could fail for other reasons, e.g. at some point we
might want to return an error when the ioctl is called for a CRTC which
is currently disabled, to avoid the hang you were getting for CRTC 1.


Dave's drm-next tree actually has a new DRM_IOCTL_GET_CAP ioctl, which
could be used for detecting this capability. It might even be possible
to handle old kernels transparently in drmWaitVBlank(), but I'm not sure
offhand if that would be better overall than doing it in the driver
code.


Some detailed technical comments:

 diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
 index 66df03c..c45abe6 100644
 --- a/src/radeon_dri2.c
 +++ b/src/radeon_dri2.c
 @@ -1145,8 +1193,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
 DrawablePtr draw,
   vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
   if (flip == 0)
   vbl.request.type |= DRM_VBLANK_NEXTONMISS;
 -if (crtc  0)
 +if (crtc == 1)
   vbl.request.type |= DRM_VBLANK_SECONDARY;
 +else if (crtc  1) {
 +   if (info-high_crtc_works) {
 +   high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT)  
 +   DRM_VBLANK_HIGH_CRTC_MASK;
 +   } else vbl.request.type |= DRM_VBLANK_SECONDARY;

Waiting on a random other CRTC makes no sense. If you know you can't
wait on the given CRTC, don't wait at all. 


 @@ -1261,6 +1319,22 @@ radeon_dri2_screen_init(ScreenPtr pScreen)
   xf86DrvMsg(pScrn-scrnIndex, X_WARNING, You need a newer kernel 
 for sync extension\n);
   }
 
 +if (info-drmmode.mode_res-count_crtcs  2) {
 +   xf86DrvMsg(pScrn-scrnIndex, X_INFO, GPU with %d CRTCs found, 
 probing VBLANKs on CRTC1\n,
 +  info-drmmode.mode_res-count_crtcs); 
 +   info-high_crtc_works = TRUE;
 +   for (c=2; cinfo-drmmode.mode_res-count_crtcs; c++) {
 +   vbl.request.type = DRM_VBLANK_RELATIVE;
 +   vbl.request.type |= (c  DRM_VBLANK_HIGH_CRTC_SHIFT)  
 DRM_VBLANK_HIGH_CRTC_MASK;
 +   vbl.request.sequence = 0;
 +   if (drmWaitVBlank(info-dri2.drm_fd, vbl)) {
 +   xf86DrvMsg(pScrn-scrnIndex, X_WARNING, Kernel rejects 
 VBLANK request on CRTC %d\n, c);
 +   info-high_crtc_works = FALSE;

Should break out of the for loop at this point. 

 +   }
 +   }
 +   if (info-high_crtc_works) xf86DrvMsg(pScrn-scrnIndex, X_INFO, 
 VBLANK request accepted on all CRTCs\n); 

The statement guarded by the if condition needs to go on a new line, and
please don't add trailing whitespace. Also, again, please match the
indentation of the surrounding code.


-- 
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: vblank problem (and proposed fix) on crtc 1

[Ilija Hadzic]

On Thu, 10 Mar 2011, Michel [ISO-8859-1] D�nzer wrote:



I'm afraid I don't really like this. Apart from the ugly bogus error
message, the probes could fail for other reasons, e.g. at some point we
might want to return an error when the ioctl is called for a CRTC which
is currently disabled, to avoid the hang you were getting for CRTC 1.



Your example actually speaks in favor of probing. Let's say that the 
future kernel starts returning an error for disabled CRTC and you don't do 
any probing. DDX will start calling vblank waits on disabled CRTC (just 
like it's doing now) and spam the kernel with errors. With probing, error 
will happen only once at screen init time.



Dave's drm-next tree actually has a new DRM_IOCTL_GET_CAP ioctl, which
could be used for detecting this capability. It might even be possible
to handle old kernels transparently in drmWaitVBlank(), but I'm not sure
offhand if that would be better overall than doing it in the driver
code.



This argument is self defeating. The purpose of the probing is to check 
for the old kernel. If I have DRM_IOCTL_GET_CAP, that means that I have a 
new kernel so probing will just work. If I have an old kernel then I don't 
have DRM_IOCTL_GET_CAP ioctl, so attemting to use it will result in an 
error from the kernel anyway. It makes no sense to me to cause an error 
in one ioctl so that you would prevent an error in other ioctl.



+   if (info-high_crtc_works) {
+   high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT) 
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   } else vbl.request.type |= DRM_VBLANK_SECONDARY;


Waiting on a random other CRTC makes no sense. If you know you can't
wait on the given CRTC, don't wait at all.



It's not random CRTC, but it's what today's DDX and DRM call secondary 
CRTC. I intentionally did this for two reasons. First, this is the 
behavior that is identical to what we see today, and I prefer to preserve 
the same behavior if either component is old (DDX or kernel), rather than 
have different behavior for different combinations of old/new. The second 
reason is pragmatic, if you want to not call wait_vblank at all and make 
sure you are safe to whatever the code above you is doing and not 
return an error you have to make up the sequence numbers and timestamps
(and probably keep track of them) and that's much more radical 
modification to the DDX. So I figured this would be conservative.



+   vbl.request.sequence = 0;
+   if (drmWaitVBlank(info-dri2.drm_fd, vbl)) {
+   xf86DrvMsg(pScrn-scrnIndex, X_WARNING, Kernel rejects VBLANK 
request on CRTC %d\n, c);
+   info-high_crtc_works = FALSE;


Should break out of the for loop at this point.



This was also intentional. I want to see in Xorg log file the full list of 
crtcs that rejected the vblank wait request. It comes handy for analyzing 
the problems and/or bugs.___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: vblank problem (and proposed fix) on crtc 1

[Michel Dänzer]

On Don, 2011-03-10 at 07:32 -0600, Ilija Hadzic wrote: 
 
 On Thu, 10 Mar 2011, Michel [ISO-8859-1] Dnzer wrote:
 
  I'm afraid I don't really like this. Apart from the ugly bogus error
  message, the probes could fail for other reasons, e.g. at some point we
  might want to return an error when the ioctl is called for a CRTC which
  is currently disabled, to avoid the hang you were getting for CRTC 1.
 
 Your example actually speaks in favor of probing. Let's say that the 
 future kernel starts returning an error for disabled CRTC and you don't do 
 any probing. DDX will start calling vblank waits on disabled CRTC (just 
 like it's doing now) and spam the kernel with errors.

No, userspace shouldn't call the ioctl for a disabled CRTC during normal
operation, that would be a bug of its own. However, a CRTC could be
disabled during the probe but get enabled later, e.g. due to
hot-plugging a display.


  Dave's drm-next tree actually has a new DRM_IOCTL_GET_CAP ioctl, which
  could be used for detecting this capability. It might even be possible
  to handle old kernels transparently in drmWaitVBlank(), but I'm not sure
  offhand if that would be better overall than doing it in the driver
  code.
 
 This argument is self defeating. The purpose of the probing is to check 
 for the old kernel. If I have DRM_IOCTL_GET_CAP, that means that I have a 
 new kernel so probing will just work. If I have an old kernel then I don't 
 have DRM_IOCTL_GET_CAP ioctl, so attemting to use it will result in an 
 error from the kernel anyway. It makes no sense to me to cause an error 
 in one ioctl so that you would prevent an error in other ioctl.

The difference is that there should be no bogus error message in dmesg,
avoiding spurious bug reports.


  +   if (info-high_crtc_works) {
  +   high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT) 
  +   DRM_VBLANK_HIGH_CRTC_MASK;
  +   } else vbl.request.type |= DRM_VBLANK_SECONDARY;
 
  Waiting on a random other CRTC makes no sense. If you know you can't
  wait on the given CRTC, don't wait at all.
 
 
 It's not random CRTC, but it's what today's DDX and DRM call secondary 
 CRTC. I intentionally did this for two reasons. First, this is the 
 behavior that is identical to what we see today, and I prefer to preserve 
 the same behavior if either component is old (DDX or kernel), rather than 
 have different behavior for different combinations of old/new.

I don't see the value in conserving fundamentally broken behaviour.

 The second reason is pragmatic, if you want to not call wait_vblank at
 all and make sure you are safe to whatever the code above you is doing
 and not return an error you have to make up the sequence numbers and
 timestamps (and probably keep track of them) and that's much more
 radical modification to the DDX.

You could probably always return 0, or previous value + 1 or whatever,
that's no more wrong than the values from a different CRTC.


  +   vbl.request.sequence = 0;
  +   if (drmWaitVBlank(info-dri2.drm_fd, vbl)) {
  +   xf86DrvMsg(pScrn-scrnIndex, X_WARNING, Kernel rejects 
  VBLANK request on CRTC %d\n, c);
  +   info-high_crtc_works = FALSE;
 
  Should break out of the for loop at this point.
 
 This was also intentional. I want to see in Xorg log file the full list of 
 crtcs that rejected the vblank wait request. It comes handy for analyzing 
 the problems and/or bugs.

Then at the very least the claim that 'an error message will appear in
the kernel only once at start up time' is wrong, there will actually be
four of them on old kernels.


-- 
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: vblank problem (and proposed fix) on crtc 1

[Ilija Hadzic]

On Thu, 10 Mar 2011, Michel [ISO-8859-1] D?nzer wrote:


No, userspace shouldn't call the ioctl for a disabled CRTC during normal
operation, that would be a bug of its own.


Well, that's exactly what DDX is doing today. Try this experiment: open a 
terminal window and run glxgears in it and observer the frame rate synced 
up to your monitor. Then in another terminal window type 'xset dpms force 
off' (or go to dpms off state any other way you want). The screen goes 
blank (disabled CRTC). Wait a while and then move your mouse to exit the 
disabled state. Notice the low frame rate reported by glxgears in one 
line. That's because glxSwapBuffers was blocked while screen was in 
power-down state.


Now I don't know if this behavior is intentional (I know your opinion 
based on the above), but it looks logical to me: if the screen is off and 
GLX attempts to wait on it, it gets what it deserves. If this is the real 
intent, then kernel returning error on disabled screen would break it (the 
application would just start to rush through rendering at max speed and 
eat up the I/O and CPU bandwidth for something that you can't see on the 
screen). Anyway, this is a topic for a separate thread.


I tend to agree that one day maybe the error is returned for some other 
reason (where your example may have been a bad one) so I will look into 
DRM_IOCTL_GET_CAP.




The difference is that there should be no bogus error message in dmesg,
avoiding spurious bug reports.



That I agree and if GET_CAP ioctl is appropriate here, I'll change to 
that. There will still be an error message in the kernel at probing time, 
but hopefully a more meaningfull one. I guess the Xorg log message should 
then be something along the you need a newer kernel lines. Agreed ?



the same behavior if either component is old (DDX or kernel), rather than
have different behavior for different combinations of old/new.


I don't see the value in conserving fundamentally broken behaviour.



There is even less value in introducing a wider variety of broken 
behaviors. Returning immediately, would cause the application to rush at 
full speed through buffer swaps and eat up the CPU and I/O bandwidth.
It think that there are equal number of ppl. who would argue for that as 
those who would argue for the opposite.




You could probably always return 0, or previous value + 1 or whatever,
that's no more wrong than the values from a different CRTC.



What I could do doesn't matter that much, because whatever I do would not 
result it fully functional system unless both the DDX and the kernel match 
in capabilities. So I go for the one that is less likely to introduce new 
bugs or unpredictable behavior.




Then at the very least the claim that 'an error message will appear in
the kernel only once at start up time' is wrong, there will actually be
four of them on old kernels.



More precisely: num_crtcs - 2 times. Use word 'once' used in a liberal 
sense ;-) it doesn't happen repetatively during the normal operation.

At least, if I can use DRM_IOCTL_GET_CAP (which I still have to check),
the message will be more meaningful.


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

vblank problem (and proposed fix) on crtc > 1

[Ilija Hadzic]

oops, just realized that I didn't include one final change to the patch I 
have just sent, so here is the real one; disregard the previous one (sorry 
wasting bandwidth).


 xf86-video-ati.patch --

diff --git a/src/radeon.h b/src/radeon.h
index 4c43717..ad80889 100644
--- a/src/radeon.h
+++ b/src/radeon.h
@@ -930,6 +930,9 @@ typedef struct {

  RADEONFBLayoutCurrentLayout;

+#ifdef RADEON_DRI2
+Bool  high_crtc_works;
+#endif
  #ifdef XF86DRI
  Bool  directRenderingEnabled;
  Bool  directRenderingInited;
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index 66df03c..c45abe6 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -783,6 +783,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 
*ust, CARD64 *msc)
  drmVBlank vbl;
  int ret;
  int crtc = radeon_dri2_drawable_crtc(draw);
+int high_crtc = 0;

  /* Drawable not displayed, make up a value */
  if (crtc == -1) {
@@ -791,8 +792,16 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 
*ust, CARD64 *msc)
  return TRUE;
  }
  vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc > 0)
+if (crtc == 1)
  vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc > 1) {
+   if (info->high_crtc_works) {
+   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
  vbl.request.sequence = 0;

  ret = drmWaitVBlank(info->dri2.drm_fd, );
@@ -825,6 +834,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
  drmVBlank vbl;
  int ret, crtc = radeon_dri2_drawable_crtc(draw);
  CARD64 current_msc;
+int high_crtc = 0;

  /* Truncate to match kernel interfaces; means occasional overflow
   * misses, but that's generally not a big deal */
@@ -855,8 +865,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,

  /* Get current count */
  vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc > 0)
+if (crtc == 1)
  vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc > 1) {
+   if (info->high_crtc_works) {
+   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & 
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
  vbl.request.sequence = 0;
  ret = drmWaitVBlank(info->dri2.drm_fd, );
  if (ret) {
@@ -882,8 +900,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
  if (current_msc >= target_msc)
  target_msc = current_msc;
  vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
-if (crtc > 0)
+if (crtc == 1)
  vbl.request.type |= DRM_VBLANK_SECONDARY;
+   else if (crtc > 1) {
+   if (info->high_crtc_works) {
+   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & 
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+   }
+   vbl.request.type |= high_crtc;
  vbl.request.sequence = target_msc;
  vbl.request.signal = (unsigned long)wait_info;
  ret = drmWaitVBlank(info->dri2.drm_fd, );
@@ -903,8 +929,15 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
   * so we queue an event that will satisfy the divisor/remainder equation.
   */
  vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
-if (crtc > 0)
+if (crtc == 1)
  vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc > 1) {
+   if (info->high_crtc_works) {
+   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & 
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   } else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;

  vbl.request.sequence = current_msc - (current_msc % divisor) +
  remainder;
@@ -1029,6 +1062,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
DrawablePtr draw,
  CARD64 current_msc;
  BoxRec box;
  RegionRec region;
+int high_crtc = 0;

  /* Truncate to match kernel interfaces; means occasional overflow
   * misses, but that's generally not a big deal */
@@ -1068,8 +1102,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
DrawablePtr draw,

  /* Get current count */
  vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc > 0)
+if (crtc == 1)
  vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc > 1) {
+   if (info->high_crtc_works) {
+   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & 
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   } else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+

vblank problem (and proposed fix) on crtc > 1

[Ilija Hadzic]

Below is an updated patch for ATI DDX (xf86-video-ati library) that
reflects the discussion of this thread. The patch is *cumulative*
(i.e., it includes the changes from a few days ago, so it should
be applied to plain-vanilla DDX, not the one you may have patched
with my patch from last week). I figure it's easier to review it 
that way, but if anyone wants an incremental patch, pls. let me know. 
The kernel patch and libdrm patch remain the same, so I am not
repeating them here.

The new thing in this patch addresses Michel's concern about spamming the 
kernel with ''Unsupported type value' when DDX is ahead of the kernel. 
However, I don't think that this can be done by checking KMS_DRIVER_MINOR 
nor DRM_IF_MINOR. The reason is that the first version number pertains to 
the device driver module (radeon.ko), and there is no change in it 
addressed by my patches. So it's inappropriate to bump up this version 
number. On the other hand, as far as I could tell I don't see a viable 
mechanism to ask the kernel for DRM_IF_MINOR (DRM_IOCTL_SET_VERSION is the 
only call that remotely resembles what would be needed and it does more 
than just querying). Also, I believe that this version number pertains to 
the interface between the drm module and the device driver, not the 
userland/kernel interface (if I am wrong, I would appreciate enlightening 
from someone who knows better).

So what I did is to actually probe the kernel at screen init time. 
When the screen is initialized, the DDX checks if the GPU has more than
two CRTCs and tries a dummy vblank wait on all crtcs > 1. If any
of them fails, it falls back to the old method of using only
primary/secondary flag. That way, an error messsage will appear
in the kernel only once at start up time and never again, which 
I think is acceptable (I am sure that at least someone will disagree,
but to me this looks like the best and the most reliable compromise). 
Anyway, it only happens when DDX is ahead of the kernel, which should be 
less.

So in summary the new behavior is that the new DDX when paired with a new
kernel, VBLANKs will work on all CRTCs the way they are supposed to. 
If any of the two components is old, the behavior is identical to the one 
we see now (VBLANKs for crtc>1, taken from crtc==1, and if that one
is disconnected, blocking of the application can happen).

-- Ilija



 patch for xf86-video-ati ---

diff --git a/src/radeon.h b/src/radeon.h
index 4c43717..ad80889 100644
--- a/src/radeon.h
+++ b/src/radeon.h
@@ -930,6 +930,9 @@ typedef struct {

  RADEONFBLayoutCurrentLayout;

+#ifdef RADEON_DRI2
+Bool  high_crtc_works;
+#endif
  #ifdef XF86DRI
  Bool  directRenderingEnabled;
  Bool  directRenderingInited;
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index 66df03c..7d77a6b 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -783,6 +783,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 
*ust, CARD64 *msc)
  drmVBlank vbl;
  int ret;
  int crtc = radeon_dri2_drawable_crtc(draw);
+int high_crtc = 0;

  /* Drawable not displayed, make up a value */
  if (crtc == -1) {
@@ -791,8 +792,16 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 
*ust, CARD64 *msc)
  return TRUE;
  }
  vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc > 0)
+if (crtc == 1)
  vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc > 1) {
+   if (info->high_crtc_works) {
+   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
  vbl.request.sequence = 0;

  ret = drmWaitVBlank(info->dri2.drm_fd, );
@@ -825,6 +834,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
  drmVBlank vbl;
  int ret, crtc = radeon_dri2_drawable_crtc(draw);
  CARD64 current_msc;
+int high_crtc = 0;

  /* Truncate to match kernel interfaces; means occasional overflow
   * misses, but that's generally not a big deal */
@@ -855,8 +865,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,

  /* Get current count */
  vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc > 0)
+if (crtc == 1)
  vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc > 1) {
+   if (info->high_crtc_works) {
+   high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & 
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
  vbl.request.sequence = 0;
  ret = drmWaitVBlank(info->dri2.drm_fd, );
  if (ret) {
@@ -882,8 +900,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
  if (current_msc >= target_msc)
  

Re: vblank problem (and proposed fix) on crtc 1

[Ilija Hadzic]

Below is an updated patch for ATI DDX (xf86-video-ati library) that
reflects the discussion of this thread. The patch is *cumulative*
(i.e., it includes the changes from a few days ago, so it should
be applied to plain-vanilla DDX, not the one you may have patched
with my patch from last week). I figure it's easier to review it 
that way, but if anyone wants an incremental patch, pls. let me know. 
The kernel patch and libdrm patch remain the same, so I am not

repeating them here.

The new thing in this patch addresses Michel's concern about spamming the 
kernel with ''Unsupported type value' when DDX is ahead of the kernel. 
However, I don't think that this can be done by checking KMS_DRIVER_MINOR 
nor DRM_IF_MINOR. The reason is that the first version number pertains to 
the device driver module (radeon.ko), and there is no change in it 
addressed by my patches. So it's inappropriate to bump up this version 
number. On the other hand, as far as I could tell I don't see a viable 
mechanism to ask the kernel for DRM_IF_MINOR (DRM_IOCTL_SET_VERSION is the 
only call that remotely resembles what would be needed and it does more 
than just querying). Also, I believe that this version number pertains to 
the interface between the drm module and the device driver, not the 
userland/kernel interface (if I am wrong, I would appreciate enlightening 
from someone who knows better).


So what I did is to actually probe the kernel at screen init time. 
When the screen is initialized, the DDX checks if the GPU has more than

two CRTCs and tries a dummy vblank wait on all crtcs  1. If any
of them fails, it falls back to the old method of using only
primary/secondary flag. That way, an error messsage will appear
in the kernel only once at start up time and never again, which 
I think is acceptable (I am sure that at least someone will disagree,
but to me this looks like the best and the most reliable compromise). 
Anyway, it only happens when DDX is ahead of the kernel, which should be 
less.


So in summary the new behavior is that the new DDX when paired with a new
kernel, VBLANKs will work on all CRTCs the way they are supposed to. 
If any of the two components is old, the behavior is identical to the one 
we see now (VBLANKs for crtc1, taken from crtc==1, and if that one

is disconnected, blocking of the application can happen).

-- Ilija



 patch for xf86-video-ati ---

diff --git a/src/radeon.h b/src/radeon.h
index 4c43717..ad80889 100644
--- a/src/radeon.h
+++ b/src/radeon.h
@@ -930,6 +930,9 @@ typedef struct {

 RADEONFBLayoutCurrentLayout;

+#ifdef RADEON_DRI2
+Bool  high_crtc_works;
+#endif
 #ifdef XF86DRI
 Bool  directRenderingEnabled;
 Bool  directRenderingInited;
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index 66df03c..7d77a6b 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -783,6 +783,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 
*ust, CARD64 *msc)
 drmVBlank vbl;
 int ret;
 int crtc = radeon_dri2_drawable_crtc(draw);
+int high_crtc = 0;

 /* Drawable not displayed, make up a value */
 if (crtc == -1) {
@@ -791,8 +792,16 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 
*ust, CARD64 *msc)
 return TRUE;
 }
 vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc  0)
+if (crtc == 1)
 vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc  1) {
+   if (info-high_crtc_works) {
+   high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT) 
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
 vbl.request.sequence = 0;

 ret = drmWaitVBlank(info-dri2.drm_fd, vbl);
@@ -825,6 +834,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
 drmVBlank vbl;
 int ret, crtc = radeon_dri2_drawable_crtc(draw);
 CARD64 current_msc;
+int high_crtc = 0;

 /* Truncate to match kernel interfaces; means occasional overflow
  * misses, but that's generally not a big deal */
@@ -855,8 +865,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,

 /* Get current count */
 vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc  0)
+if (crtc == 1)
 vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc  1) {
+   if (info-high_crtc_works) {
+	high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT)  
+		DRM_VBLANK_HIGH_CRTC_MASK;

+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
 vbl.request.sequence = 0;
 ret = drmWaitVBlank(info-dri2.drm_fd, vbl);
 if (ret) {
@@ -882,8 +900,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
 if (current_msc = target_msc)
 target_msc = current_msc;
 

Re: vblank problem (and proposed fix) on crtc 1

[Ilija Hadzic]

oops, just realized that I didn't include one final change to the patch I 
have just sent, so here is the real one; disregard the previous one (sorry 
wasting bandwidth).



 xf86-video-ati.patch --

diff --git a/src/radeon.h b/src/radeon.h
index 4c43717..ad80889 100644
--- a/src/radeon.h
+++ b/src/radeon.h
@@ -930,6 +930,9 @@ typedef struct {

 RADEONFBLayoutCurrentLayout;

+#ifdef RADEON_DRI2
+Bool  high_crtc_works;
+#endif
 #ifdef XF86DRI
 Bool  directRenderingEnabled;
 Bool  directRenderingInited;
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index 66df03c..c45abe6 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -783,6 +783,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 
*ust, CARD64 *msc)
 drmVBlank vbl;
 int ret;
 int crtc = radeon_dri2_drawable_crtc(draw);
+int high_crtc = 0;

 /* Drawable not displayed, make up a value */
 if (crtc == -1) {
@@ -791,8 +792,16 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 
*ust, CARD64 *msc)
 return TRUE;
 }
 vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc  0)
+if (crtc == 1)
 vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc  1) {
+   if (info-high_crtc_works) {
+   high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT) 
+   DRM_VBLANK_HIGH_CRTC_MASK;
+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
 vbl.request.sequence = 0;

 ret = drmWaitVBlank(info-dri2.drm_fd, vbl);
@@ -825,6 +834,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
 drmVBlank vbl;
 int ret, crtc = radeon_dri2_drawable_crtc(draw);
 CARD64 current_msc;
+int high_crtc = 0;

 /* Truncate to match kernel interfaces; means occasional overflow
  * misses, but that's generally not a big deal */
@@ -855,8 +865,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,

 /* Get current count */
 vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc  0)
+if (crtc == 1)
 vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc  1) {
+   if (info-high_crtc_works) {
+	high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT)  
+		DRM_VBLANK_HIGH_CRTC_MASK;

+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
 vbl.request.sequence = 0;
 ret = drmWaitVBlank(info-dri2.drm_fd, vbl);
 if (ret) {
@@ -882,8 +900,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
 if (current_msc = target_msc)
 target_msc = current_msc;
 vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
-if (crtc  0)
+if (crtc == 1)
 vbl.request.type |= DRM_VBLANK_SECONDARY;
+   else if (crtc  1) {
+   if (info-high_crtc_works) {
+		high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT)  
+		DRM_VBLANK_HIGH_CRTC_MASK;

+   }
+   else vbl.request.type |= DRM_VBLANK_SECONDARY;
+   }
+   vbl.request.type |= high_crtc;
 vbl.request.sequence = target_msc;
 vbl.request.signal = (unsigned long)wait_info;
 ret = drmWaitVBlank(info-dri2.drm_fd, vbl);
@@ -903,8 +929,15 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, 
DrawablePtr draw,
  * so we queue an event that will satisfy the divisor/remainder equation.
  */
 vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
-if (crtc  0)
+if (crtc == 1)
 vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc  1) {
+   if (info-high_crtc_works) {
+	high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT)  
+		DRM_VBLANK_HIGH_CRTC_MASK;

+   } else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;

 vbl.request.sequence = current_msc - (current_msc % divisor) +
 remainder;
@@ -1029,6 +1062,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
DrawablePtr draw,
 CARD64 current_msc;
 BoxRec box;
 RegionRec region;
+int high_crtc = 0;

 /* Truncate to match kernel interfaces; means occasional overflow
  * misses, but that's generally not a big deal */
@@ -1068,8 +1102,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
DrawablePtr draw,

 /* Get current count */
 vbl.request.type = DRM_VBLANK_RELATIVE;
-if (crtc  0)
+if (crtc == 1)
 vbl.request.type |= DRM_VBLANK_SECONDARY;
+else if (crtc  1) {
+   if (info-high_crtc_works) {
+	high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT)  
+		DRM_VBLANK_HIGH_CRTC_MASK;

+   } else vbl.request.type |= DRM_VBLANK_SECONDARY;
+}
+vbl.request.type |= high_crtc;
 vbl.request.sequence = 0;
 ret = drmWaitVBlank(info-dri2.drm_fd, vbl);
 if (ret) {
@@ -,8 +1152,15 @@ static int 

vblank problem (and proposed fix) on crtc > 1

[Michel Dänzer]

On Fre, 2011-03-04 at 19:27 -0600, Ilija Hadzic wrote: 
> 
> That will take care of ATI DDX and general support in DRM; I presume
> that someone will follow up on other DDXs (I only deal with Radeon
> GPUs at the moment, so I am not familiar with other DDXs).

Are there any other GPUs with more than 2 CRTCs (and KMS drivers) yet?


-- 
Earthling Michel D?nzer   |http://www.vmware.com
Libre software enthusiast |  Debian, X and DRI developer

Re: vblank problem (and proposed fix) on crtc 1

[Michel Dänzer]

On Fre, 2011-03-04 at 19:27 -0600, Ilija Hadzic wrote: 
 
 That will take care of ATI DDX and general support in DRM; I presume
 that someone will follow up on other DDXs (I only deal with Radeon
 GPUs at the moment, so I am not familiar with other DDXs).

Are there any other GPUs with more than 2 CRTCs (and KMS drivers) yet?


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

vblank problem (and proposed fix) on crtc > 1

[Ilija Hadzic]

(this is a cumulative response to all comments that came in on this 
thread).

My opinion is that extending the existing ioctl is better than introducing 
the new one given that they will be doing the same thing. Also there are 
fewer kernel changes so it's safer (it opens fewer opportunities to screw 
up and it will be easier to review and vet the changes). We probably 
shouldn't start the new vs. not-new ioctl debate, since even those who 
advocate the former seem to agree that my proposal is a pragmatic one.

I agree that it is not a good idea to "spam" the old kernel with 
'unsupported type value', so I'll add a hook to the DDX to check the 
version and not issue the ioctl at all if it is requested on a higher 
CRTC. I think that's better than falling back to the old style and issuing 
the system call on (potentially wrong) CRTC #1 because that can block the 
application (and I'd rather see it proceed without attempting vblank 
synchronization, then block).

I'll modify my patches to include the above stated check and retest when I 
get a little time and post them to this list. In the meantime, if anyone 
wants to venture into using the patches as I sent yesterday, please report 
the experience. On my end things seem to be working well, my specific 
problems are solved and it doesn't look like I broke anything, but more 
ppl. test, the better. That will take care of ATI DDX and general support 
in DRM; I presume that someone will follow up on other DDXs (I only deal 
with Radeon GPUs at the moment, so I am not familiar with other DDXs).

-- Ilija

vblank problem (and proposed fix) on crtc > 1

[Michel Dänzer]

On Don, 2011-03-03 at 17:34 -0600, Ilija Hadzic wrote: 
> 
> Now the problem is that DRM_IOCTL_WAIT_VBLANK only understands primary
> and secondary crtc and everything that is not crtc-0 is considered
> secondary. Then in the kernel, drm module maps the secondary flag to 
> crtc 1, but that is a disconnected crtc on which VBLANK interrupts never
> come.
> 
> I am under impression that this limitation is a legacy from the days
> when GPUs had at most two connectors.

At most two CRTCs, but yeah.


My initial reaction to your proposal was similar to Jesse's, that there
should be a new, cleaner ioctl for this instead. However, it looks like
this can actually be done without making the existing ioctl too much
messier than it is already. Given this and that it'll be hard to make a
new ioctl perfect or at least cleanly extensible in the future, it may
make sense to take this approach.


> In case that DDX is ahead of the kernel and tries to use the high_crtc
> field, kernel will return -EINVAL (due to failed mask check), but 
> the application will progress without waiting on VBLANK, which is
> still better than being stuck as it is now.

However, this will result in the kernel output getting spammed with
'Unsupported type value' error messages, won't it? One way to prevent
this would be to bump include/drm/drm_core.h:DRM_IF_MINOR or
drivers/gpu/drm/radeon/radeon_drv.c:KMS_DRIVER_MINOR and check for that
before using the extended functionality in userspace. That would also
prevent the hangs.


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


> --- xf86-video-ati (DDX) patch 
> -
> 
> diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> index 66df03c..5cbe544 100644
> --- a/src/radeon_dri2.c
> +++ b/src/radeon_dri2.c
> @@ -1068,8 +1087,12 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
> DrawablePtr draw,
> 
>   /* Get current count */
>   vbl.request.type = DRM_VBLANK_RELATIVE;
> -if (crtc > 0)
> +if (crtc == 1)
>   vbl.request.type |= DRM_VBLANK_SECONDARY;
> +else if (crtc > 1)
> + high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & 
> + DRM_VBLANK_HIGH_CRTC_MASK;
> +vbl.request.type |= high_crtc;
>   vbl.request.sequence = 0;
>   ret = drmWaitVBlank(info->dri2.drm_fd, );
>   if (ret) {

Please try to preserve the indentation of surrounding code.


-- 
Earthling Michel D?nzer   |http://www.vmware.com
Libre software enthusiast |  Debian, X and DRI developer

Re: vblank problem (and proposed fix) on crtc 1

[Michel Dänzer]

On Don, 2011-03-03 at 17:34 -0600, Ilija Hadzic wrote: 
 
 Now the problem is that DRM_IOCTL_WAIT_VBLANK only understands primary
 and secondary crtc and everything that is not crtc-0 is considered
 secondary. Then in the kernel, drm module maps the secondary flag to 
 crtc 1, but that is a disconnected crtc on which VBLANK interrupts never
 come.
 
 I am under impression that this limitation is a legacy from the days
 when GPUs had at most two connectors.

At most two CRTCs, but yeah.


My initial reaction to your proposal was similar to Jesse's, that there
should be a new, cleaner ioctl for this instead. However, it looks like
this can actually be done without making the existing ioctl too much
messier than it is already. Given this and that it'll be hard to make a
new ioctl perfect or at least cleanly extensible in the future, it may
make sense to take this approach.


 In case that DDX is ahead of the kernel and tries to use the high_crtc
 field, kernel will return -EINVAL (due to failed mask check), but 
 the application will progress without waiting on VBLANK, which is
 still better than being stuck as it is now.

However, this will result in the kernel output getting spammed with
'Unsupported type value' error messages, won't it? One way to prevent
this would be to bump include/drm/drm_core.h:DRM_IF_MINOR or
drivers/gpu/drm/radeon/radeon_drv.c:KMS_DRIVER_MINOR and check for that
before using the extended functionality in userspace. That would also
prevent the hangs.


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


 --- xf86-video-ati (DDX) patch 
 -
 
 diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
 index 66df03c..5cbe544 100644
 --- a/src/radeon_dri2.c
 +++ b/src/radeon_dri2.c
 @@ -1068,8 +1087,12 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
 DrawablePtr draw,
 
   /* Get current count */
   vbl.request.type = DRM_VBLANK_RELATIVE;
 -if (crtc  0)
 +if (crtc == 1)
   vbl.request.type |= DRM_VBLANK_SECONDARY;
 +else if (crtc  1)
 + high_crtc = (crtc  DRM_VBLANK_HIGH_CRTC_SHIFT)  
 + DRM_VBLANK_HIGH_CRTC_MASK;
 +vbl.request.type |= high_crtc;
   vbl.request.sequence = 0;
   ret = drmWaitVBlank(info-dri2.drm_fd, vbl);
   if (ret) {

Please try to preserve the indentation of surrounding code.


-- 
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: vblank problem (and proposed fix) on crtc 1

[Ilija Hadzic]

(this is a cumulative response to all comments that came in on this 
thread).


My opinion is that extending the existing ioctl is better than introducing 
the new one given that they will be doing the same thing. Also there are 
fewer kernel changes so it's safer (it opens fewer opportunities to screw 
up and it will be easier to review and vet the changes). We probably 
shouldn't start the new vs. not-new ioctl debate, since even those who 
advocate the former seem to agree that my proposal is a pragmatic one.


I agree that it is not a good idea to spam the old kernel with 
'unsupported type value', so I'll add a hook to the DDX to check the 
version and not issue the ioctl at all if it is requested on a higher 
CRTC. I think that's better than falling back to the old style and issuing 
the system call on (potentially wrong) CRTC #1 because that can block the 
application (and I'd rather see it proceed without attempting vblank 
synchronization, then block).


I'll modify my patches to include the above stated check and retest when I 
get a little time and post them to this list. In the meantime, if anyone 
wants to venture into using the patches as I sent yesterday, please report 
the experience. On my end things seem to be working well, my specific 
problems are solved and it doesn't look like I broke anything, but more 
ppl. test, the better. That will take care of ATI DDX and general support 
in DRM; I presume that someone will follow up on other DDXs (I only deal 
with Radeon GPUs at the moment, so I am not familiar with other DDXs).


-- Ilija

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

vblank problem (and proposed fix) on crtc > 1

[Ilija Hadzic]

I would like to propose an extension of the interface between the 
libdrm and drm kernel module for VBLANK wait to address a problem that
I am seeing on screens with more than two displays. Below, I describe the 
problem, propose a (backwards compatible) solution and provide a set
of patches that implement it. I am looking for some feedback on whether 
this would be a reasonable and acceptable fix. Sorry for the length of this 
note.

Specific setup I use is Radeon HD5870 (Evergreen/Cypress) GPU in Zaphod 
mode with three displays (DVI-0, DVI-1 and DisplayPort-0), but the
problem applies to any configuration with more than two displays. 
A sample xorg.conf is pasted at the end of this note for reference. 
Everything works fine if all three displays are connected to the 
monitor.

However, if I yank out DVI-1 connector, leaving DVI-0 and DisplayPort-0
plugged in and restart X without changing the xorg.conf (fairly legitimate
action of an ordinary user) any application that needs VBLANK synchronization
(e.g. glxgears) will get stuck when started in the desktop associated with
DisplayPort-0. It will still progress on DVI-0 and DVI-1 is (of course) not
visible so it doesn't matter.

Tracing the userland and the kernel, I have found out that the reason this
happens is because in this configuration DisplayPort-0 gets assigned to 
crtc-2 (crtc-0 is on DVI-0 and crtc-1 is still on unconnected DVI-1). 
Now the problem is that DRM_IOCTL_WAIT_VBLANK only understands primary
and secondary crtc and everything that is not crtc-0 is considered
secondary. Then in the kernel, drm module maps the secondary flag to 
crtc 1, but that is a disconnected crtc on which VBLANK interrupts never
come.

I am under impression that this limitation is a legacy from the days
when GPUs had at most two connectors. However, analyzing the whole
VBLANK mechanism in the kernel, it looks like it is ready supporting 
VBLANKs on higher crtc numbers (at least I can tell that with confidence
for radeon driver, which is the one I am using as well as for the 
drm module itself; didn't check other GPUs). Also, it looks like
at least when DRI2 is used, the userland fully preserves CRTC 
information all the way from GLX/mesa down to the call into drmWaitVBlank, 
which is the function in libdrm that issues the ioctl.

Specifically in case of DRI2, all calls into drmWaitVBlank are done 
through three functions: radeon_dri2_get_msc, radeon_dri2_schedule_wait_msc 
and radeon_dri2_schedule_swap in DDX (xf86-video-ati library). These 
functions have information about the exact CRTC in which
the drawable is, but they "destroy" it by mapping it to primary/secondary 
flag in the vbl.request.type. In the kernel, the flag is mapped back
to crtc number (0 or 1), so crtc > 1 is never used.

The fix/improvement I propose is to extend the request.type field
in drmVBlank structure with additional 5 bits that I call high_crtc
(there are lots of unused bits in that field). 5 bits covers for 32
CRTCs, which seems to be the hard limit imposed by various parts of the 
Xorg and DDX (e.g. possible_crtcs mask in the display descriptor, 
and the like). If high_crtc is zero, then DRM (kernel module) 
looks at the primary/secondary flag and maps them to crtc 0 and 1
(backwards compatibility with older DDX or DDX for other device 
that does not use the new high_crtc field). If it's not zero then it 
means that the higher CRTC number is specified by DDX (i.e. userland 
is a new DDX) and vblank is waited on the specified CRTC (this is used
only for crtc > 1, crtc 0 and 1, still use the old style).

In case that DDX is ahead of the kernel and tries to use the high_crtc
field, kernel will return -EINVAL (due to failed mask check), but 
the application will progress without waiting on VBLANK, which is
still better than being stuck as it is now. On the other hand, 
if DDX that is ahead of the kernel uses CRTC 0 or 1, this won't 
cause the old kernel to complain because these two CRTCs would
still use the old-style with primary/secondary flag.

So the solution is in my opinion as graceful towards as it can be 
towards old kernel and fully backwards-compatible with old DDX.

Things seem to test very well on my machines (I am currently working 
with GPUs with many displays, so I care about this support a lot), at
least when it comes to using Radeon cards. I would like some feedback 
on whether there could be any issues that I am overseeing and also
whether it would break anyone else's DDX. I had a private conversation
with Alex Deucher and he seems to be fine as far as Radeon GPUs are 
concerned. What do other folks think ? I believe that eliminating
the above-described limitation has to start at least with some GPU
and others will follow if it works out well.

There is also a potential issue a few places in Mesa that call 
drmWaitVBlank directly from there (and they lose the CRTC 
information quite early in the call stack), but I don't think 
they are used at all in DRI2 case (so we can 

vblank problem (and proposed fix) on crtc > 1

[Jesse Barnes]

On Thu, 3 Mar 2011 17:34:53 -0600 (CST)
Ilija Hadzic  wrote:

> The fix/improvement I propose is to extend the request.type field
> in drmVBlank structure with additional 5 bits that I call high_crtc
> (there are lots of unused bits in that field). 5 bits covers for 32
> CRTCs, which seems to be the hard limit imposed by various parts of the 
> Xorg and DDX (e.g. possible_crtcs mask in the display descriptor, 
> and the like). If high_crtc is zero, then DRM (kernel module) 
> looks at the primary/secondary flag and maps them to crtc 0 and 1
> (backwards compatibility with older DDX or DDX for other device 
> that does not use the new high_crtc field). If it's not zero then it 
> means that the higher CRTC number is specified by DDX (i.e. userland 
> is a new DDX) and vblank is waited on the specified CRTC (this is used
> only for crtc > 1, crtc 0 and 1, still use the old style).

Yeah, I think that should work, though another option would be to just
add a new ioctl.  That would make compat checking easy for new code; it
could just call the new ioctl and if that returned -ENOTTY it could
fall back to the old one and throw away the CRTC info or complain if
the count was too high.

But you're right that when we re-wrote the code we fixed it to handle >
2 CRTCs, so it should be mostly ready for that (modulo testing, which
it sounds like you're doing already).

Jesse

vblank problem (and proposed fix) on crtc 1

[Ilija Hadzic]

I would like to propose an extension of the interface between the 
libdrm and drm kernel module for VBLANK wait to address a problem that
I am seeing on screens with more than two displays. Below, I describe the 
problem, propose a (backwards compatible) solution and provide a set
of patches that implement it. I am looking for some feedback on whether 
this would be a reasonable and acceptable fix. Sorry for the length of this 
note.


Specific setup I use is Radeon HD5870 (Evergreen/Cypress) GPU in Zaphod 
mode with three displays (DVI-0, DVI-1 and DisplayPort-0), but the
problem applies to any configuration with more than two displays. 
A sample xorg.conf is pasted at the end of this note for reference. 
Everything works fine if all three displays are connected to the 
monitor.


However, if I yank out DVI-1 connector, leaving DVI-0 and DisplayPort-0
plugged in and restart X without changing the xorg.conf (fairly legitimate
action of an ordinary user) any application that needs VBLANK synchronization
(e.g. glxgears) will get stuck when started in the desktop associated with
DisplayPort-0. It will still progress on DVI-0 and DVI-1 is (of course) not
visible so it doesn't matter.

Tracing the userland and the kernel, I have found out that the reason this
happens is because in this configuration DisplayPort-0 gets assigned to 
crtc-2 (crtc-0 is on DVI-0 and crtc-1 is still on unconnected DVI-1). 
Now the problem is that DRM_IOCTL_WAIT_VBLANK only understands primary

and secondary crtc and everything that is not crtc-0 is considered
secondary. Then in the kernel, drm module maps the secondary flag to 
crtc 1, but that is a disconnected crtc on which VBLANK interrupts never

come.

I am under impression that this limitation is a legacy from the days
when GPUs had at most two connectors. However, analyzing the whole
VBLANK mechanism in the kernel, it looks like it is ready supporting 
VBLANKs on higher crtc numbers (at least I can tell that with confidence
for radeon driver, which is the one I am using as well as for the 
drm module itself; didn't check other GPUs). Also, it looks like
at least when DRI2 is used, the userland fully preserves CRTC 
information all the way from GLX/mesa down to the call into drmWaitVBlank, 
which is the function in libdrm that issues the ioctl.


Specifically in case of DRI2, all calls into drmWaitVBlank are done 
through three functions: radeon_dri2_get_msc, radeon_dri2_schedule_wait_msc 
and radeon_dri2_schedule_swap in DDX (xf86-video-ati library). These 
functions have information about the exact CRTC in which
the drawable is, but they destroy it by mapping it to primary/secondary 
flag in the vbl.request.type. In the kernel, the flag is mapped back

to crtc number (0 or 1), so crtc  1 is never used.

The fix/improvement I propose is to extend the request.type field
in drmVBlank structure with additional 5 bits that I call high_crtc
(there are lots of unused bits in that field). 5 bits covers for 32
CRTCs, which seems to be the hard limit imposed by various parts of the 
Xorg and DDX (e.g. possible_crtcs mask in the display descriptor, 
and the like). If high_crtc is zero, then DRM (kernel module) 
looks at the primary/secondary flag and maps them to crtc 0 and 1
(backwards compatibility with older DDX or DDX for other device 
that does not use the new high_crtc field). If it's not zero then it 
means that the higher CRTC number is specified by DDX (i.e. userland 
is a new DDX) and vblank is waited on the specified CRTC (this is used

only for crtc  1, crtc 0 and 1, still use the old style).

In case that DDX is ahead of the kernel and tries to use the high_crtc
field, kernel will return -EINVAL (due to failed mask check), but 
the application will progress without waiting on VBLANK, which is
still better than being stuck as it is now. On the other hand, 
if DDX that is ahead of the kernel uses CRTC 0 or 1, this won't 
cause the old kernel to complain because these two CRTCs would

still use the old-style with primary/secondary flag.

So the solution is in my opinion as graceful towards as it can be 
towards old kernel and fully backwards-compatible with old DDX.


Things seem to test very well on my machines (I am currently working 
with GPUs with many displays, so I care about this support a lot), at
least when it comes to using Radeon cards. I would like some feedback 
on whether there could be any issues that I am overseeing and also

whether it would break anyone else's DDX. I had a private conversation
with Alex Deucher and he seems to be fine as far as Radeon GPUs are 
concerned. What do other folks think ? I believe that eliminating

the above-described limitation has to start at least with some GPU
and others will follow if it works out well.

There is also a potential issue a few places in Mesa that call 
drmWaitVBlank directly from there (and they lose the CRTC 
information quite early in the call stack), but I don't think 
they are used at all in DRI2 case 

Re: vblank problem (and proposed fix) on crtc 1

[Jesse Barnes]

On Thu, 3 Mar 2011 17:34:53 -0600 (CST)
Ilija Hadzic ihad...@research.bell-labs.com wrote:

 The fix/improvement I propose is to extend the request.type field
 in drmVBlank structure with additional 5 bits that I call high_crtc
 (there are lots of unused bits in that field). 5 bits covers for 32
 CRTCs, which seems to be the hard limit imposed by various parts of the 
 Xorg and DDX (e.g. possible_crtcs mask in the display descriptor, 
 and the like). If high_crtc is zero, then DRM (kernel module) 
 looks at the primary/secondary flag and maps them to crtc 0 and 1
 (backwards compatibility with older DDX or DDX for other device 
 that does not use the new high_crtc field). If it's not zero then it 
 means that the higher CRTC number is specified by DDX (i.e. userland 
 is a new DDX) and vblank is waited on the specified CRTC (this is used
 only for crtc  1, crtc 0 and 1, still use the old style).

Yeah, I think that should work, though another option would be to just
add a new ioctl.  That would make compat checking easy for new code; it
could just call the new ioctl and if that returned -ENOTTY it could
fall back to the old one and throw away the CRTC info or complain if
the count was too high.

But you're right that when we re-wrote the code we fixed it to handle 
2 CRTCs, so it should be mostly ready for that (modulo testing, which
it sounds like you're doing already).

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