Re: [PATCH 2/3] drm: Prevent invalid use of vblank_disable_immediate.

["Michel Dänzer" <michel@daenzer.net>]

On 15.04.2015 02:41, Mario Kleiner wrote:
> For a kms driver to support immediate disable of vblank
> irq's reliably without introducing off by one errors or
> other mayhem for clients, it must not only support a
> hardware vblank counter query, but also high precision
> vblank timestamping, so vblank count and timestamp can be
> instantaneously reinitialzed to valid values. Additionally
> the exposed hardware counter must behave as if it is
> incrementing at leading edge of vblank to avoid off by
> one errors during reinitialization of the counter while
> the display happens to be inside or close to vblank.
> 
> Check during drm_vblank_init that a driver which claims to
> be capable of vblank_disable_immediate at least supports
> high precision timestamping and prevent use of instant
> disable if that isn't present as a minimum requirement.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index af9662e..6efe822 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -336,6 +336,12 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
>  	else
>  		DRM_INFO("No driver support for vblank timestamp query.\n");
>  
> +	/* Must have precise timestamping for reliable vblank instant disable */
> +	if (dev->vblank_disable_immediate && !dev->driver->get_vblank_timestamp) {
> +		dev->vblank_disable_immediate = false;
> +		DRM_ERROR("Set vblank_disable_immediate, but not supported.\n");
> +	}

I think DRM_ERROR is kind of a bad compromise for this. If this is
considered a driver bug, something like WARN_ONCE would be better to
draw attention to the culprit. Otherwise, maybe something like

DRM_INFO("Setting vblank_disable_immediate to false because "
	 "get_vblank_timestamp == NULL\n");

would be both clearer and less alarming.

Other than that, looks good to me.


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