Hi Laurent,
On 03/03/17 02:17, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Wednesday 01 Mar 2017 13:12:56 Kieran Bingham wrote:
>> Updating the state in a running VSP1 requires two interrupts from the
>> VSP. Initially, the updated state will be committed - but only after the
>> VSP1 has completed processing it's current frame will the new state be
>> taken into account. As such, the committed state will only be 'completed'
>> after an extra frame completion interrupt.
>>
>> Track this delay, by passing the frame flip event through the VSP
>> module; It will be returned only when the frame has completed and can be
>> returned to the caller.
>
> I'll check the interrupt sequence logic tomorrow, it's a bit too late now.
> Please see below for additional comments.
No worries
>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 8 +++++-
>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 1 +-
>> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 34 ++++++++++++++++++++++++++-
>> 3 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 7391dd95c733..0a824633a012
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -328,7 +328,7 @@ static bool rcar_du_crtc_page_flip_pending(struct
>> rcar_du_crtc *rcrtc) bool pending;
>>
>> spin_lock_irqsave(&dev->event_lock, flags);
>> - pending = rcrtc->event != NULL;
>> + pending = (rcrtc->event != NULL) || (rcrtc->pending != NULL);
>> spin_unlock_irqrestore(&dev->event_lock, flags);
>>
>> return pending;
>> @@ -578,6 +578,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>> rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);
>>
>> if (status & DSSR_FRM) {
>> +
>> + if (rcrtc->pending) {
>> + trace_printk("VBlank loss due to VSP Overrun\n");
>> + return IRQ_HANDLED;
>> + }
>> +
>> drm_crtc_handle_vblank(&rcrtc->crtc);
>> rcar_du_crtc_finish_page_flip(rcrtc);
>> ret = IRQ_HANDLED;
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index a7194812997e..8374a858446a
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> @@ -46,6 +46,7 @@ struct rcar_du_crtc {
>> bool started;
>>
>> struct drm_pending_vblank_event *event;
>> + struct drm_pending_vblank_event *pending;
>> wait_queue_head_t flip_wait;
>>
>> unsigned int outputs;
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 71e70e1e0881..408375aff1a0
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> @@ -28,6 +28,26 @@
>> #include "rcar_du_kms.h"
>> #include "rcar_du_vsp.h"
>>
>> +static void rcar_du_vsp_complete(void *private, void *data)
>> +{
>> + struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
>> + struct drm_device *dev = crtc->crtc.dev;
>> + struct drm_pending_vblank_event *event;
>> + bool match;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&dev->event_lock, flags);
>> + event = crtc->event;
>> + crtc->event = data;
>> + match = (crtc->event == crtc->pending);
>> + crtc->pending = NULL;
>> + spin_unlock_irqrestore(&dev->event_lock, flags);
>> +
>> + /* Safety checks */
>> + WARN(event, "Event lost by VSP completion callback\n");
>> + WARN(!match, "Stored pending event, does not match completion\n");
>
> I understand you want to be safe, and I assume these have never been triggered
> in your tests. I'd rather replace them by a mechanism that doesn't require
> passing the event to the VSP driver, and that wouldn't require adding a
> pending field to the rcar_du_crtc structure.
Ok - understandable, I started this way - but hit problems, which I think were
unrelated. Anyway, I switched to 'moving' the event so that I could be sure
rcar_du_crtc_finish_page_flip() couldn't have an event to send.
I can switch back to keeping it's 'ownership' in rcar-du.
> Wouldn't adding a WARN_ON(rcrtc-
>> event) in rcar_du_crtc_atomic_begin() in the if (crtc->state->event) block do
> the job ?
Yes, that would :D - I put those in just to be sure things were going as
expected, and there weren't any code paths I hadn't considered. But I think they
are 'unreachable' warnings anyway (at least at the moment).
> Would this get in the way of your trace_printk() debugging in
> rcar_du_crtc_irq() ? Could you implement the debugging in a separate patch
> without requiring to pass the event to the VSP driver ?
Moving the event wasn't necessarily about trace_printk debugging, but perhaps it
was part of me debugging why I hadn't got things working correctly.
Moving the event out meant I was certain it couldn't let userspace flip until
after the VSP had returned the event. I can adjust this now.
So the next issue is in the performance, which I am currently investigating:
We are currently halving the throughput due to requiring two interrupts on any
update in the VSP1.
We have interrupt timings such as the following:
(loops)
VSP-FRE : | | | | |
0uS
VSP-DFE : | | | | |
4uS
VSP-DST : | | | | |
58uS
DU-DSSR : | | | | |
16187uS
(From the timings shown in http://paste.ubuntu.com/24089285/)
Currently, an atomic flush committed between A-B, commences at B, and completes
at C.
VSP-FRE : | | | | |
VSP-DFE : | A B C |
VSP-DST : | | | | |
DU-DSSR : | |F | | |
The atomic flush occurs immediately after, DU-DSSR has 'flipped' a page, at 'F'
in the above diagram.
This gives plenty of time for processing the lists, but of course means that
actually processing is a full frame latency behind.
Full timing log can be seen at http://paste.ubuntu.com/24101386/
--
Regards
Kieran
>> +}
>> +
>> void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>> {
>> const struct drm_display_mode *mode = &crtc->crtc.state-
>> adjusted_mode;
>> @@ -66,6 +86,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>> */
>> crtc->group->need_restart = true;
>>
>> + vsp1_du_register_callback(crtc->vsp->vsp, rcar_du_vsp_complete, crtc);
>> +
>> vsp1_du_setup_lif(crtc->vsp->vsp, mode->hdisplay, mode->vdisplay);
>> }
>>
>> @@ -81,7 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>>
>> void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>> {
>> - vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
>> + struct drm_device *dev = crtc->crtc.dev;
>> + struct drm_pending_vblank_event *event;
>> + unsigned long flags;
>> +
>> + /* Move the event to the VSP, track it locally as 'pending' */
>> + spin_lock_irqsave(&dev->event_lock, flags);
>> + event = crtc->pending = crtc->event;
>> + crtc->event = NULL;
>> + spin_unlock_irqrestore(&dev->event_lock, flags);
>> +
>> + vsp1_du_atomic_flush(crtc->vsp->vsp, event);
>> }
>>
>> /* Keep the two tables in sync. */
>
