Hi Daniel,
On 29.09.2016 11:44, Daniel Vetter wrote:
> On Tue, Sep 27, 2016 at 03:36:14PM +0200, Andrzej Hajda wrote:
>> A lot of drivers need to fire pageflip completion event at very next vblank
>> interrupt. The patch adds helper to perform this operation.
>> drm_crtc_arm_completion_event checks if there is an event to handle,
>> if vblank reference get succeeds it arms the event, otherwise it sends the
>> event immediately.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_irq.c | 25 +++++++++++++++++++++++++
>> include/drm/drm_irq.h | 1 +
>> 2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 77f357b..313d323 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -1053,6 +1053,31 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>> EXPORT_SYMBOL(drm_crtc_send_vblank_event);
>>
>> /**
>> + * drm_crtc_arm_completion_event - conditionally arm crtc completion event
>> + * @crtc: the source CRTC of the completion event
>> + *
>> + * A lot of drivers need to fire pageflip completion event at very next vblank
>> + * interrupt. This helper tries to arm the event in case of successful vblank
>> + * get otherwise it sends the event immediately.
> This function is copypasted tons of times over all drivers because they
> were broken, and this hack at least got the events working. But in general
> this here is racy, and if Mario ever runs on your hw he'll get upset about
> the wrong timings.
Could you explain what is racy here? I am not vblank expert, but the code
for me looked more or less OK.
>
> If we go with this (and I'm really not convinced it's a good idea) then it
> should have real big warning that you should never use this in a new
> driver.
It looked to me as an easy copy/paste cleaning :)
Anyway I would like to have correct solution, at least in case of exynos.
>
>> + */
>> +void drm_crtc_arm_completion_event(struct drm_crtc *crtc)
>> +{
>> + struct drm_pending_vblank_event *event = crtc->state->event;
>> +
>> + if (event) {
>> + crtc->state->event = NULL;
>> +
>> + spin_lock_irq(&crtc->dev->event_lock);
>> + if (drm_crtc_vblank_get(crtc) == 0)
> This check here completely torpedoes the design principle of atomic
> helpers that drivers always know the state of the crtc. Again I only did
> this because I didn't want to buy hw&test it for all the drivers which got
> this wrong.
If you mean driver should know that getting vblank should be possible
or not this code could be probably parametrized, for example:
drm_crtc_handle_completion_event(struct drm_crtc *crtc, bool send_immediately)
>> + drm_crtc_arm_vblank_event(crtc, event);
>> + else
>> + drm_crtc_send_vblank_event(crtc, event);
>> + spin_unlock_irq(&crtc->dev->event_lock);
>> + }
>> +}
>> +EXPORT_SYMBOL(drm_crtc_arm_completion_event);
> Maybe we need an EXPORT_SYMBOL_BROKEN for this .... btw the kerneldoc of
> the functions you're called do explain that this is not as easy as it
> seems. That's also something you throw under the rug here.
> -Daniel
After quick look I have not found these kerneldocs, where can read about it.
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
