Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function

From: "Michel Dänzer" <michel@daenzer.net>
To: "Thierry Reding" <thierry.reding@gmail.com>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function
Date: Thu, 31 Jul 2014 10:14:02 +0900	[thread overview]
Message-ID: <53D9985A.1050202@daenzer.net> (raw)
In-Reply-To: <20140730152135.GD1345@ulmo>

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

On 31.07.2014 00:21, Thierry Reding wrote:
> On Wed, Jul 30, 2014 at 05:36:21PM +0300, Ville Syrjälä wrote:
>> On Wed, Jul 30, 2014 at 04:20:25PM +0200, Thierry Reding wrote:
>>> On Wed, Jul 30, 2014 at 05:32:28PM +0900, Michel Dänzer wrote:
>>>> On 30.07.2014 17:22, Daniel Vetter wrote:
>>>>> On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
>>>>>> On 30.07.2014 06:32, Daniel Vetter wrote:
>>>>>>> + * due to lack of driver support or because the crtc is off.
>>>>>>> + */
>>>>>>> +void drm_crtc_vblank_wait(struct drm_crtc *crtc)
>>>>>>> +{
>>>>>>> +	drm_vblank_wait(crtc->dev, drm_crtc_index(crtc));
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(drm_crtc_vblank_wait);
>>>>>>> +
>>>>>>> +/**
>>>>>>
>>>>>> Maybe the function names should be *_vblank_wait_next() or something to
>>>>>> clarify the purpose and reduce potential confusion versus drm_wait_vblank().
>>>>>
>>>>> Yeah that name is just transferred from the i915 driver. What about
>>>>> drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?
>>>>
>>>> I don't care that much :), go ahead.
>>>
>>> Just my two cents: our downstream kernel has a helper somewhat like this
>>> which waits for a specified number of frames (apparently this is useful
>>> for some panels that require up to 5 or 6 frames before they display the
>>> correct image on screen). So perhaps something like this could work:
>>>
>>> 	void drm_wait_vblank_count(struct drm_device *dev, unsigned int crtc,
>>> 				   unsigned int count)
>>> 	{
>>> 		u32 last;
>>> 		int ret;
>>>
>>> 		ret = drm_vblank_get(dev, crtc);
>>> 		if (WARN_ON(ret))
>>> 			return;
>>>
>>> 		while (count--) {
>>> 			last = drm_vblank_count(dev, crtc);
>>>
>>> 			...
>>> 		}
>>>
>>> 		drm_vblank_put(dev, crtc);
>>> 	}
>>
>> Would be nicer to wait for an absolute vblank count instead IMO. Or
>> if you want to pass a relative count in just convert it to an absolute
>> count first and wait for it (taking wraparound into account obviously).
> 
> Hmm... would something like this work?
> 
> 	target = drm_vblank_count(dev, crtc) + count;
> 
> 	ret = wait_event_timeout(...,
> 				 drm_vblank_count(dev, crtc) == target,
> 				 ...);
> 
> That should properly take into account wrap-around given that both sites
> use drm_vblank_count().

I think it would be better to refactor drm_wait_vblank() than to
reinvent it.

-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 234 bytes --]

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

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