[PATCH v4 6/9] drm/tegra: Implement page-flipping support

[Mario Kleiner]

On 02/25/2013 08:06 AM, Thierry Reding wrote:
> On Fri, Feb 22, 2013 at 05:13:47PM +0100, Mario Kleiner wrote:
>> On 02/21/2013 03:35 PM, Thierry Reding wrote:
>>> All the necessary support bits like .mode_set_base() and VBLANK are now
>>> available, so page-flipping case easily be implemented on top.
>>>
>>> Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>
>>> ---
>>> Changes in v3:
>>> - use drm_send_vblank_event()
>>> - set crtc->fb field
>>>
>>> Changes in v4:
>>> - fix a potential race by checking that a framebuffer base has been
>>>    latched when completing a page-flip
>>>
>>>   drivers/gpu/drm/tegra/dc.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/tegra/dc.h  |  2 ++
>>>   drivers/gpu/drm/tegra/drm.c |  9 +++++++
>>>   drivers/gpu/drm/tegra/drm.h |  5 ++++
>>>   4 files changed, 82 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>> index 5f55a25..c5d825f 100644
>>> --- a/drivers/gpu/drm/tegra/dc.c
>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>> @@ -183,7 +183,72 @@ void tegra_dc_disable_vblank(struct tegra_dc *dc)
>>>   	spin_unlock_irqrestore(&dc->lock, flags);
>>>   }
>>> +static void tegra_dc_finish_page_flip(struct tegra_dc *dc)
>>> +{
>>> +	struct drm_device *drm = dc->base.dev;
>>> +	struct drm_crtc *crtc = &dc->base;
>>> +	struct drm_gem_cma_object *gem;
>>> +	unsigned long flags, base;
>>> +
>>
>> The checks for properly latched scanout look good to me and should
>> fix the race between software and hardware. But shouldn't you
>> already spin_lock_irqsave() here, before checking dc->event and
>> performing write access on the DC_CMD_STATE_ACCESS registers? And
>> lock around most of the tegra_dc_page_flip() code below, like in
>> your previous iteration of the patch, to prevent a race between
>> pageflip ioctl and the vblank irq handler?
>
> Currently there's nothing else that touches the DC_CMD_STATE_ACCESS
> register, so that part should be safe.
>

ok.

> As to the locking that I removed since the previous patch, I looked at
> it more closely and didn't think it was necessary. Also nothing in my
> tests seemed to point to any race here, though I probably didn't cover
> everything.
>
>>> +	if (!dc->event)
>>
>> -> !dc->event may exit prematurely because the irq handler on one
>> core doesn't notice the new setup of dc->event by
>> tegra_dc_page_flip() on a different core? Don't know if that can
>> happen, don't know the memory ordering of arm, but could imagine
>> that this or the compiler reordering instructions could cause
>> trouble without lock protection.
>
> I'm not sure I follow here. If the handler sees a NULL here, why would
> it want to continue? It can safely assume that the page flip wasn't
> setup for this interval, can't it?
>
> If indeed the VBLANK interrupt happens exactly around the assignment to
> dc->event, then we'll just schedule the page-flip for the next VBLANK.
>

I meant the other way round. dc->event is initially NULL. Then pageflip 
ioctl is called, close to vblank, the code e.g., executing on core 1. if 
(event) branch in tegra_dc_page_flip() executes, assigns dc->event = 
event etc. calls tegra_dc_set_base() before onset of vblank, pageflip 
completes in vblank, but the irq handler and thereby 
tegra_dc_finish_page_flip() running on a different core doesn't notice 
dc->event is non-NULL, because the changes haven't propagated to the 
different core without some memory write barrier etc., thereby doesn't 
run the flip completion in the vblank of actual flip completion. But i 
now think my point is probably pointless, because the successive calls 
to drm_vblank_get() and tegra_dc_set_base() contain enough locking and 
implicit memory barriers that such a stale value there won't happen.


>>> +		return;
>>> +
>>> +	gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
>> -> crtc->fb gets updated in tegra_dc_page_flip() after
>> tegra_dc_set_base() without lock -> possibly stale fb here?
>
> Good point. That may indeed require a lock. I'll need to look more
> closely.
>

Yep, i think here my argument from above may hold for crtc->fb, nothing 
here to prevent a race afaics.

>>> +	/* check if new start address has been latched */
>>> +	tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
>>> +	base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
>>> +	tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
>>> +
>>
>> -> Can other code in the driver write to DC_CMD_STATE_ACCESS,
>> simultaneously to tegra_dc_page_flip writing->reading->writing and
>> cause trouble?
>
> This is the only location where the DC_CMD_STATE_ACCESS register is
> actually used in the driver so we should be safe for now.
>

Ok. I'm just paranoid about forgetting such things on later changes.


>>> +static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>>> +			      struct drm_pending_vblank_event *event)
>>> +{
>>> +	struct tegra_dc *dc = to_tegra_dc(crtc);
>>> +	struct drm_device *drm = crtc->dev;
>>> +
>>> +	if (dc->event)
>>> +		return -EBUSY;
>>> +
>>
>> -> Lock already here?
>>
>>> +	if (event) {
>>> +		event->pipe = dc->pipe;
>>> +		dc->event = event;
>>> +		drm_vblank_get(drm, dc->pipe);
>>> +	}
>>> +
>>> +	tegra_dc_set_base(dc, 0, 0, fb);
>>> +	crtc->fb = fb;
>>> +
>>
>> -> Unlock here?
>
> I tried to expand the lock after we discussed this previously but it
> caused the code to deadlock. While in the process of tracking it down I
> decided that the lock wasn't actually required at all.
>
> But I hadn't thought about the stale FB issue, so I'll check again. I
> notice that Dave has already merged this series, so if nobody objects
> I'll fix it up in a follow-up patch.
>

Fine with me.

thanks,
-mario