diff mbox

[7/8] drm/irq: Implement a generic vblank_wait function

Message ID 1406669543-31213-8-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter July 29, 2014, 9:32 p.m. UTC
As usual in both a crtc index and a struct drm_crtc * version.

The function assumes that no one drivers their display below 10Hz, and
it will complain if the vblank wait takes longer than that.

v2: Also check dev->max_vblank_counter since some drivers register a
fake get_vblank_counter function.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h        |  2 ++
 2 files changed, 47 insertions(+)

Comments

Michel Dänzer July 30, 2014, 2:59 a.m. UTC | #1
On 30.07.2014 06:32, Daniel Vetter wrote:
> As usual in both a crtc index and a struct drm_crtc * version.
> 
> The function assumes that no one drivers their display below 10Hz, and
> it will complain if the vblank wait takes longer than that.
> 
> v2: Also check dev->max_vblank_counter since some drivers register a
> fake get_vblank_counter function.

What does that refer to? Can't find any other reference to
max_vblank_counter in the patch.


> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 0de123afdb34..76024fdde452 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
>  EXPORT_SYMBOL(drm_crtc_vblank_put);
>  
>  /**
> + * drm_vblank_wait - wait for one vblank
> + * @dev: DRM device
> + * @crtc: crtc index
> + *
> + * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
> + * It is a failure to call this when the vblank irq for @crtc is disable, e.g.

Spelling: 'disabled'


> + * due to lack of driver support or because the crtc is off.
> + */
> +void drm_vblank_wait(struct drm_device *dev, int crtc)
> +{
> +	int ret;
> +	u32 last;
> +
> +	ret = drm_vblank_get(dev, crtc);
> +	if (WARN_ON(ret))
> +		return;
> +
> +	last = drm_vblank_count(dev, crtc);
> +
> +#define C (last != drm_vblank_count(dev, crtc))
> +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> +				 C, msecs_to_jiffies(100));
> +
> +	WARN_ON(ret == 0);
> +#undef C

What's the point of the C macro?


> +	drm_vblank_put(dev, crtc);
> +}
> +EXPORT_SYMBOL(drm_vblank_wait);
> +
> +/**
> + * drm_crtc_vblank_wait - wait for one vblank
> + * @crtc: DRM crtc
> + *
> + * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
> + * It is a failure to call this when the vblank irq for @crtc is disable, e.g.

Same typo as above.


> + * 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().


Looks good to me other than that.
Daniel Vetter July 30, 2014, 8:22 a.m. UTC | #2
On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
> On 30.07.2014 06:32, Daniel Vetter wrote:
> > As usual in both a crtc index and a struct drm_crtc * version.
> > 
> > The function assumes that no one drivers their display below 10Hz, and
> > it will complain if the vblank wait takes longer than that.
> > 
> > v2: Also check dev->max_vblank_counter since some drivers register a
> > fake get_vblank_counter function.
> 
> What does that refer to? Can't find any other reference to
> max_vblank_counter in the patch.

Oops, that was from an intermediate version. The v3: text somehow got lost
where I've switched the code from directly calling the ->get_counter
callback to using drm_vblank_counter, which will dtrt even when there's
not hw counter available. Will augment the commit message.

> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 0de123afdb34..76024fdde452 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
> >  EXPORT_SYMBOL(drm_crtc_vblank_put);
> >  
> >  /**
> > + * drm_vblank_wait - wait for one vblank
> > + * @dev: DRM device
> > + * @crtc: crtc index
> > + *
> > + * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
> > + * It is a failure to call this when the vblank irq for @crtc is disable, e.g.
> 
> Spelling: 'disabled'
> 
> 
> > + * due to lack of driver support or because the crtc is off.
> > + */
> > +void drm_vblank_wait(struct drm_device *dev, int crtc)
> > +{
> > +	int ret;
> > +	u32 last;
> > +
> > +	ret = drm_vblank_get(dev, crtc);
> > +	if (WARN_ON(ret))
> > +		return;
> > +
> > +	last = drm_vblank_count(dev, crtc);
> > +
> > +#define C (last != drm_vblank_count(dev, crtc))
> > +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> > +				 C, msecs_to_jiffies(100));
> > +
> > +	WARN_ON(ret == 0);
> > +#undef C
> 
> What's the point of the C macro?

Usually the conditions tend to overflow the 80 char limit, so I've adopted
that pattern of extracting it into a local #define. I think it'll fit
here, so will roll in.

> 
> 
> > +	drm_vblank_put(dev, crtc);
> > +}
> > +EXPORT_SYMBOL(drm_vblank_wait);
> > +
> > +/**
> > + * drm_crtc_vblank_wait - wait for one vblank
> > + * @crtc: DRM crtc
> > + *
> > + * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
> > + * It is a failure to call this when the vblank irq for @crtc is disable, e.g.
> 
> Same typo as above.
> 
> 
> > + * 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()? At least to my ear
vblank_wait_next sounds backwards.

> Looks good to me other than that.

If you're ok with the name suggestion I'll polish & resend, thanks for the
comments.
-Daniel
Michel Dänzer July 30, 2014, 8:32 a.m. UTC | #3
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.
Thierry Reding July 30, 2014, 2:20 p.m. UTC | #4
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);
	}

Then implement drm_wait_vblank() (or drm_wait_one_vblank()) on top of
that as a special case. Of course one could equally well implement
drm_wait_vblank_count() on top of your drm_wait_{one_,}vblank().

I couldn't think of a safe way to make the above work without the
loop...

Thierry
Thierry Reding July 30, 2014, 2:24 p.m. UTC | #5
On Tue, Jul 29, 2014 at 11:32:22PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
[...]
> +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> +				 C, msecs_to_jiffies(100));

100 milliseconds looks like a very arbitrary choice. Theoretically we
support things like crtc->mode.vrefresh = 5, so I wonder if we should
parameterize this on the refresh rate, or simply increase it further?

Thierry
Ville Syrjälä July 30, 2014, 2:36 p.m. UTC | #6
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).
Daniel Vetter July 30, 2014, 3:06 p.m. UTC | #7
On Wed, Jul 30, 2014 at 04:24:06PM +0200, Thierry Reding wrote:
> On Tue, Jul 29, 2014 at 11:32:22PM +0200, Daniel Vetter wrote:
> [...]
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> [...]
> > +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> > +				 C, msecs_to_jiffies(100));
> 
> 100 milliseconds looks like a very arbitrary choice. Theoretically we
> support things like crtc->mode.vrefresh = 5, so I wonder if we should
> parameterize this on the refresh rate, or simply increase it further?

Once we have 10Hz panels I think we can pump/fix this. In i915 we've used
50ms everywhere, and never hit this. Except when driver bugs, ofc.
-Daniel
Daniel Vetter July 30, 2014, 3:07 p.m. UTC | #8
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).

Yeah I've conisidered to to a generic version, but don't though about all
the ways I'll get wrap-around wrong and decided that we better postpone
this until there's a real need. We can easily extract it later on ...
-Daniel
Thierry Reding July 30, 2014, 3:10 p.m. UTC | #9
On Wed, Jul 30, 2014 at 05:06:38PM +0200, Daniel Vetter wrote:
> On Wed, Jul 30, 2014 at 04:24:06PM +0200, Thierry Reding wrote:
> > On Tue, Jul 29, 2014 at 11:32:22PM +0200, Daniel Vetter wrote:
> > [...]
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > [...]
> > > +	ret = wait_event_timeout(dev->vblank[crtc].queue,
> > > +				 C, msecs_to_jiffies(100));
> > 
> > 100 milliseconds looks like a very arbitrary choice. Theoretically we
> > support things like crtc->mode.vrefresh = 5, so I wonder if we should
> > parameterize this on the refresh rate, or simply increase it further?
> 
> Once we have 10Hz panels I think we can pump/fix this. In i915 we've used
> 50ms everywhere, and never hit this. Except when driver bugs, ofc.

Okay, fair enough.

Thierry
Thierry Reding July 30, 2014, 3:21 p.m. UTC | #10
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().

Thierry
Michel Dänzer July 31, 2014, 1:14 a.m. UTC | #11
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.
Daniel Vetter July 31, 2014, 7:54 a.m. UTC | #12
On Thu, Jul 31, 2014 at 3:14 AM, Michel Dänzer <michel@daenzer.net> wrote:
> I think it would be better to refactor drm_wait_vblank() than to
> reinvent it.

That's the ioctl implementation which spends most of its time decoding
ioctl structures. If we take that out then there's about half a line
which would be shared (since a lot of the stuff in there is ums gunk
that we don't want to carry over to a kms-only internal interface). So
imo that doesn't make sense.
-Daniel
Michel Dänzer July 31, 2014, 8:56 a.m. UTC | #13
On 31.07.2014 16:54, Daniel Vetter wrote:
> On Thu, Jul 31, 2014 at 3:14 AM, Michel Dänzer <michel@daenzer.net> wrote:
>> I think it would be better to refactor drm_wait_vblank() than to
>> reinvent it.
> 
> That's the ioctl implementation which spends most of its time decoding
> ioctl structures. If we take that out then there's about half a line
> which would be shared (since a lot of the stuff in there is ums gunk
> that we don't want to carry over to a kms-only internal interface). So
> imo that doesn't make sense.

I'm referring to the core logic of waiting for a number of vblank
periods or until the vblank period with a given sequence number, dealing
with wraparound etc. The issues you guys were discussing for a new
function were ironed out there long ago.
Daniel Vetter July 31, 2014, 9:46 a.m. UTC | #14
On Thu, Jul 31, 2014 at 10:56 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 31.07.2014 16:54, Daniel Vetter wrote:
>> On Thu, Jul 31, 2014 at 3:14 AM, Michel Dänzer <michel@daenzer.net> wrote:
>>> I think it would be better to refactor drm_wait_vblank() than to
>>> reinvent it.
>>
>> That's the ioctl implementation which spends most of its time decoding
>> ioctl structures. If we take that out then there's about half a line
>> which would be shared (since a lot of the stuff in there is ums gunk
>> that we don't want to carry over to a kms-only internal interface). So
>> imo that doesn't make sense.
>
> I'm referring to the core logic of waiting for a number of vblank
> periods or until the vblank period with a given sequence number, dealing
> with wraparound etc. The issues you guys were discussing for a new
> function were ironed out there long ago.

I'm referering to the same, but that logic is gunked up with
special-cases for UMS and absolute vblank waits and all kinds of other
stuff, so that sharing this with a kms helper to wait a few vblanks
(so relative only) really doesn't buy us all that much. Actually I
think you'll be left with nothing shared since for the kms
driver-internal functions really shouldn't have all these hacks to
paper over races and other issues (like ums shutting down the pipe
while we didn't look).
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 0de123afdb34..76024fdde452 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -999,6 +999,51 @@  void drm_crtc_vblank_put(struct drm_crtc *crtc)
 EXPORT_SYMBOL(drm_crtc_vblank_put);
 
 /**
+ * drm_vblank_wait - wait for one vblank
+ * @dev: DRM device
+ * @crtc: crtc index
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disable, e.g.
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_vblank_wait(struct drm_device *dev, int crtc)
+{
+	int ret;
+	u32 last;
+
+	ret = drm_vblank_get(dev, crtc);
+	if (WARN_ON(ret))
+		return;
+
+	last = drm_vblank_count(dev, crtc);
+
+#define C (last != drm_vblank_count(dev, crtc))
+	ret = wait_event_timeout(dev->vblank[crtc].queue,
+				 C, msecs_to_jiffies(100));
+
+	WARN_ON(ret == 0);
+#undef C
+
+	drm_vblank_put(dev, crtc);
+}
+EXPORT_SYMBOL(drm_vblank_wait);
+
+/**
+ * drm_crtc_vblank_wait - wait for one vblank
+ * @crtc: DRM crtc
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disable, e.g.
+ * 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);
+
+/**
  * drm_vblank_off - disable vblank events on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 06a673894c47..f72e5ef5f7b0 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1355,6 +1355,8 @@  extern int drm_vblank_get(struct drm_device *dev, int crtc);
 extern void drm_vblank_put(struct drm_device *dev, int crtc);
 extern int drm_crtc_vblank_get(struct drm_crtc *crtc);
 extern void drm_crtc_vblank_put(struct drm_crtc *crtc);
+extern void drm_vblank_wait(struct drm_device *dev, int crtc);
+extern void drm_crtc_vblank_wait(struct drm_crtc *crtc);
 extern void drm_vblank_off(struct drm_device *dev, int crtc);
 extern void drm_vblank_on(struct drm_device *dev, int crtc);
 extern void drm_crtc_vblank_off(struct drm_crtc *crtc);