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

I'd suggest making _DRM_VBLANK_HIGH_CRTC_SHIFT either 21 (so
_DRM_VBLANK_HIGH_CRTC_MASK is adjacent to _DRM_VBLANK_EVENT) or much
lower, say 8 or even 4, as the flags are more likely to get extended
than the types, if history is any indication.

Also,

#define _DRM_VBLANK_HIGH_CRTC_MASK (0x1F << _DRM_VBLANK_HIGH_CRTC_SHIFT)

would make it obvious how these values are related and decrease the
likelihood of divergence during development of the patch.


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