Re: [RFC PATCH 3/3] drm: rcar-du: Register a completion callback with VSP1

2017-03-03 Thread Kieran Bingham 
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 
>> ---
>>  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(>event_lock, flags);
>> -pending = rcrtc->event != NULL;
>> +pending = (rcrtc->event != NULL) || (rcrtc->pending != NULL);
>>  spin_unlock_irqrestore(>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(>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(>event_lock, flags);
>> +event = crtc->event;
>> +crtc->event = data;
>> +match = (crtc->event == crtc->pending);
>> +crtc->pending = NULL;
>> +spin_unlock_irqrestore(>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

Re: [RFC PATCH 3/3] drm: rcar-du: Register a completion callback with VSP1

2017-03-02 Thread Laurent Pinchart 
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.

> Signed-off-by: Kieran Bingham 
> ---
>  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(>event_lock, flags);
> - pending = rcrtc->event != NULL;
> + pending = (rcrtc->event != NULL) || (rcrtc->pending != NULL);
>   spin_unlock_irqrestore(>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(>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(>event_lock, flags);
> + event = crtc->event;
> + crtc->event = data;
> + match = (crtc->event == crtc->pending);
> + crtc->pending = NULL;
> + spin_unlock_irqrestore(>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. Wouldn't adding a WARN_ON(rcrtc-
>event) in rcar_du_crtc_atomic_begin() in the if (crtc->state->event) block do 
the job ?

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 ?

> +}
> +
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  {
>   const struct drm_display_mode *mode = >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

[RFC PATCH 3/3] drm: rcar-du: Register a completion callback with VSP1

2017-03-01 Thread Kieran Bingham 
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.

Signed-off-by: Kieran Bingham 
---
 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(>event_lock, flags);
-   pending = rcrtc->event != NULL;
+   pending = (rcrtc->event != NULL) || (rcrtc->pending != NULL);
spin_unlock_irqrestore(>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(>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(>event_lock, flags);
+   event = crtc->event;
+   crtc->event = data;
+   match = (crtc->event == crtc->pending);
+   crtc->pending = NULL;
+   spin_unlock_irqrestore(>event_lock, flags);
+
+   /* Safety checks */
+   WARN(event, "Event lost by VSP completion callback\n");
+   WARN(!match, "Stored pending event, does not match completion\n");
+}
+
 void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 {
const struct drm_display_mode *mode = >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(>event_lock, flags);
+   event = crtc->pending = crtc->event;
+   crtc->event = NULL;
+   spin_unlock_irqrestore(>event_lock, flags);
+
+   vsp1_du_atomic_flush(crtc->vsp->vsp, event);
 }
 
 /* Keep the two tables in sync. */
-- 
git-series 0.9.1