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
