date:20170304

[PATCH v2] drm: rcar-du: Arm the page flip event after queuing the page flip

2017-03-04 Thread Laurent Pinchart

The page flip event is armed in the atomic begin handler, creating a
race condition with the frame end interrupt that could send the event
before the atomic operation actually completes. To avoid that, arm the
event in the atomic flush handler after queuing the page flip.

This change doesn't fully close the race window, as the frame end
interrupt could be generated before the page flip is committed to
hardware but only handled after the event is armed. However, the race
window is now much smaller.

The event must however be armed before calling the VSP atomic commit
function, otherwise the completion callback could arrive before we arm
the event, resulting in a deadlock.

Signed-off-by: Laurent Pinchart 
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

Changes since v1:

- Arm the event before calling the VSP atomic commit function

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 7391dd95c733..2aceb84fc15d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -502,17 +502,6 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc 
*crtc,
  struct drm_crtc_state *old_crtc_state)
 {
struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
-   struct drm_device *dev = rcrtc->crtc.dev;
-   unsigned long flags;
-
-   if (crtc->state->event) {
-   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-
-   spin_lock_irqsave(>event_lock, flags);
-   rcrtc->event = crtc->state->event;
-   crtc->state->event = NULL;
-   spin_unlock_irqrestore(>event_lock, flags);
-   }
 
if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
rcar_du_vsp_atomic_begin(rcrtc);
@@ -522,9 +511,20 @@ static void rcar_du_crtc_atomic_flush(struct drm_crtc 
*crtc,
  struct drm_crtc_state *old_crtc_state)
 {
struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+   struct drm_device *dev = rcrtc->crtc.dev;
+   unsigned long flags;
 
rcar_du_crtc_update_planes(rcrtc);
 
+   if (crtc->state->event) {
+   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+
+   spin_lock_irqsave(>event_lock, flags);
+   rcrtc->event = crtc->state->event;
+   crtc->state->event = NULL;
+   spin_unlock_irqrestore(>event_lock, flags);
+   }
+
if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
rcar_du_vsp_atomic_flush(rcrtc);
 }
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l: vsp1: Postpone page flip in event of display list race

2017-03-04 Thread Laurent Pinchart

Hi Kieran,

Thank you for the patch.

On Saturday 04 Mar 2017 02:01:18 Kieran Bingham wrote:
> If we try to commit the display list while an update is pending, we have
> missed our opportunity. The display list manager will hold the commit
> until the next interrupt.
> 
> In this event, we inform the vsp1 completion callback handler so that
> the du will not perform a page flip out of turn.
> 
> Signed-off-by: Kieran Bingham 
> ---
>  drivers/media/platform/vsp1/vsp1_dl.c   |  9 +++--
>  drivers/media/platform/vsp1/vsp1_dl.h   |  2 +-
>  drivers/media/platform/vsp1/vsp1_drm.c  |  4 +++-
>  drivers/media/platform/vsp1/vsp1_pipe.c |  6 +-
>  drivers/media/platform/vsp1/vsp1_pipe.h |  2 ++
>  5 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index ad545aff4e35..f8e8c90f22bc
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -557,9 +557,10 @@ void vsp1_dlm_irq_display_start(struct vsp1_dl_manager
> *dlm)
>   spin_unlock(>lock);
>  }
> 
> -void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> +int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
>   struct vsp1_device *vsp1 = dlm->vsp1;
> + int ret = 0;
> 
>   spin_lock(>lock);
> 
> @@ -578,8 +579,10 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager
> *dlm) * before interrupt processing. The hardware hasn't taken the update *
> into account yet, we'll thus skip one frame and retry.
>*/
> - if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD)
> + if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD) {
> + ret = -EBUSY;

Getting there, but not quite :-)

What we need to protect against is the display list not being committed early 
enough, resulting in one frame skip in the hardware. The good news is that the 
driver already detects the opposite condition just below. 

>   goto done;
> + }
> 
>   /* The device starts processing the queued display list right after 
the
>* frame end interrupt. The display list thus becomes active.

This is what we want to report. You can simply return a bool that will tell 
whether the previous display list has completed at frame end. You should 
return true when the condition right below this comment is true, as well as 
when using header mode (to avoid breaking mem-to-mem pipelines), and false in 
all other cases.

> @@ -606,6 +609,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> 
>  done:
>   spin_unlock(>lock);
> +
> + return ret;
>  }
> 
>  /* Hardware Setup */
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
> b/drivers/media/platform/vsp1/vsp1_dl.h index 7131aa3c5978..c772a1d92513
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -28,7 +28,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device
> *vsp1, void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
>  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
>  void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm);
> -void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
> +int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
> 
>  struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
>  void vsp1_dl_list_put(struct vsp1_dl_list *dl);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 85e5ebca82a5..6f2dd42ca01b
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -40,10 +40,12 @@ static void vsp1_du_pipeline_frame_end(struct
> vsp1_pipeline *pipe) {
>   struct vsp1_drm *drm = to_vsp1_drm(pipe);
> 
> - if (drm->du_complete && drm->du_pending) {
> + if (drm->du_complete && drm->du_pending && !pipe->dl_postponed) {
>   drm->du_complete(drm->du_private);
>   drm->du_pending = false;
>   }
> +
> + pipe->dl_postponed = false;
>  }
> 
>  /* 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 280ba0804699..3c5aae8767dd
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -297,10 +297,14 @@ bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe)
> 
>  void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
>  {
> + int ret;
> +
>   if (pipe == NULL)
>   return;
> 
> - vsp1_dlm_irq_frame_end(pipe->output->dlm);
> + ret = vsp1_dlm_irq_frame_end(pipe->output->dlm);
> + if (ret)
> + pipe->dl_postponed = true;

This can be simplified greatly. If vsp1_dlm_irq_frame_end() returns false, 
return immediately without calling the pipe->frame_end() handler or 
incrementing the sequence number, as the frame

[PATCH] drm: rcar-du: Remove wait field from rcar_du_device structure

2017-03-04 Thread Laurent Pinchart

The field is a left-over from the switch to the atomic commit helper.
It's unused, remove it.

Signed-off-by: Laurent Pinchart 
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 --
 drivers/gpu/drm/rcar-du/rcar_du_drv.h | 5 -
 2 files changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c 
b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index decad036b031..eb905e6b5f4c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -328,8 +328,6 @@ static int rcar_du_probe(struct platform_device *pdev)
if (rcdu == NULL)
return -ENOMEM;
 
-   init_waitqueue_head(>commit.wait);
-
rcdu->dev = >dev;
rcdu->info = of_device_get_match_data(rcdu->dev);
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h 
b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index c843c3134498..574b3c1c21df 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -98,11 +98,6 @@ struct rcar_du_device {
unsigned int vspd1_sink;
 
struct rcar_du_lvdsenc *lvds[RCAR_DU_MAX_LVDS];
-
-   struct {
-   wait_queue_head_t wait;
-   u32 pending;
-   } commit;
 };
 
 static inline bool rcar_du_has(struct rcar_du_device *rcdu,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] v4l: vsp1: extend VSP1 module API to allow DRM callbacks

2017-03-04 Thread Laurent Pinchart

Hi Kieran,

Thank you for the patch.

On Saturday 04 Mar 2017 02:01:17 Kieran Bingham wrote:
> To be able to perform page flips in DRM without flicker we need to be
> able to notify the rcar-du module when the VSP has completed its
> processing.
> 
> We must not have bidirectional dependencies on the two components to
> maintain support for loadable modules, thus we extend the API to allow
> a callback to be registered within the VSP DRM interface.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> v2:
>  - vsp1_du_setup_lif() uses config structure to set callbacks
>  - vsp1_du_pipeline_frame_end() moved to interrupt section
>  - vsp1_du_pipeline_frame_end registered in vsp1_drm_init()
>meaning of any NULL values
>  - removed unnecessary 'private data' variables
> 
>  drivers/media/platform/vsp1/vsp1_drm.c | 20 
>  drivers/media/platform/vsp1/vsp1_drm.h | 10 ++
>  include/media/vsp1.h   |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 7dce55043379..85e5ebca82a5
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -36,6 +36,16 @@ void vsp1_drm_display_start(struct vsp1_device *vsp1)
>   vsp1_dlm_irq_display_start(vsp1->drm->pipe.output->dlm);
>  }
> 
> +static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
> +{
> + struct vsp1_drm *drm = to_vsp1_drm(pipe);
> +
> + if (drm->du_complete && drm->du_pending) {
> + drm->du_complete(drm->du_private);
> + drm->du_pending = false;
> + }
> +}
> +
>  /* 
>   * DU Driver API
>   */
> @@ -95,6 +105,7 @@ int vsp1_du_setup_lif(struct device *dev, const struct
> vsp1_du_lif_config *cfg)
>   }
> 
>   pipe->num_inputs = 0;
> + vsp1->drm->du_complete = NULL;
> 
>   vsp1_dlm_reset(pipe->output->dlm);
>   vsp1_device_put(vsp1);
> @@ -196,6 +207,13 @@ int vsp1_du_setup_lif(struct device *dev, const struct
> vsp1_du_lif_config *cfg) if (ret < 0)
>   return ret;
> 
> + /*
> +  * Register a callback to allow us to notify the DRM framework of 
frame
> +  * completion events.
> +  */
> + vsp1->drm->du_complete = cfg->callback;
> + vsp1->drm->du_private = cfg->callback_data;
> +
>   ret = media_pipeline_start(>output->entity.subdev.entity,
> >pipe);
>   if (ret < 0) {
> @@ -504,6 +522,7 @@ void vsp1_du_atomic_flush(struct device *dev)
> 
>   vsp1_dl_list_commit(pipe->dl);
>   pipe->dl = NULL;
> + vsp1->drm->du_pending = true;
> 
>   /* Start or stop the pipeline if needed. */
>   if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> @@ -597,6 +616,7 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
>   pipe->lif = >lif->entity;
>   pipe->output = vsp1->wpf[0];
>   pipe->output->pipe = pipe;
> + pipe->frame_end = vsp1_du_pipeline_frame_end;
> 
>   return 0;
>  }
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> b/drivers/media/platform/vsp1/vsp1_drm.h index 9e28ab9254ba..3a53e9a60c73
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -33,8 +33,18 @@ struct vsp1_drm {
>   struct v4l2_rect compose;
>   unsigned int zpos;
>   } inputs[VSP1_MAX_RPF];
> +
> + /* Frame syncronisation */
> + void (*du_complete)(void *);
> + void *du_private;

These fields need to be documented.

> + bool du_pending;

You can remove the du_pending flag, see my comments in patch 2/3.

>  };
> 
> +static inline struct vsp1_drm *to_vsp1_drm(struct vsp1_pipeline *pipe)
> +{
> + return container_of(pipe, struct vsp1_drm, pipe);
> +}
> +
>  int vsp1_drm_init(struct vsp1_device *vsp1);
>  void vsp1_drm_cleanup(struct vsp1_device *vsp1);
>  int vsp1_drm_create_links(struct vsp1_device *vsp1);
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index bfc701f04f3f..f6629f19f209 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -23,6 +23,9 @@ int vsp1_du_init(struct device *dev);
>  struct vsp1_du_lif_config {
>   unsigned int width;
>   unsigned int height;
> +
> + void (*callback)(void *);
> + void *callback_data;

These fields need to be documented too.

>  };
> 
>  int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config
> *cfg);

-- 
Regards,

Laurent Pinchart

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

2017-03-04 Thread Laurent Pinchart

Hi Kieran,

On Saturday 04 Mar 2017 15:07:09 Laurent Pinchart wrote:
> On Saturday 04 Mar 2017 02:01:19 Kieran Bingham wrote:
> > Currently we process page flip events on every display interrupt,
> > however this does not take into consideration the processing time needed
> > by the VSP1 utilised in the pipeline.
> > 
> > Register a callback with the VSP driver to obtain completion events, and
> > track them so that we only perform page flips when the full display
> > pipeline has completed for the frame.
> > 
> > Signed-off-by: Kieran Bingham 
> > 
> > ---
> > 
> > v2:
> >  - Commit message completely re-worded for patch re-work.
> >  - drm_crtc_handle_vblank() re-instated in event of rcrtc->pending
> >  - removed passing of unnecessary 'data' through callbacks
> >  - perform page flips from the VSP completion handler
> >  - add locking around pending flags
> >  
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 +++--
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++-
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 29 +++-
> >  3 files changed, 39 insertions(+), 2 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b0ff304ce3dc..1fcd311badb1
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c

[snip]

> > @@ -28,6 +28,22 @@
> >  #include "rcar_du_kms.h"
> >  #include "rcar_du_vsp.h"
> > 
> > +static void rcar_du_vsp_complete(void *private)
> > +{
> > +   struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
> > +   struct drm_device *dev = crtc->crtc.dev;
> > +   unsigned long flags;
> > +   bool pending;
> > +
> > +   spin_lock_irqsave(>event_lock, flags);
> > +   pending = crtc->pending;
> > +   crtc->pending = false;
> > +   spin_unlock_irqrestore(>event_lock, flags);
> > +
> > +   if (pending)
> > +   rcar_du_crtc_finish_page_flip(crtc);
> 
> This seems to duplicate the synchronization mechanism based on events in
> rcar_du_crtc_atomic_begin(). I need to check that in more details.

Indeed it does, and I don't think that's needed. You might be able to shorten 
the race window on the DU side, but you won't be able to close it completely 
as detecting the race requires information that is only available to the VSP 
driver. Fixing the race on the VSP side will make this dead code, so you can 
remove the addition of the pending flag from this patch.

> > +}

-- 
Regards,

Laurent Pinchart

[PATCH v6 1/3] watchdog: add rza_wdt driver

2017-03-04 Thread Chris Brandt

Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
handler is also included since a WDT overflow is the only method for
restarting an RZ/A SoC.

Signed-off-by: Chris Brandt 
---
v3:
* added #include 
* alphabetized #include list
* removed call to platform_set_drvdata
v2:
* removed extra lines from file header comments
* changed (1<<#) to BIT(#)
* changed #define WTSCR_CKS(i) i to (i)
* changed format to #define SOMETHINGvalue (and align values)
* now check if clock rate < 16384
* added space before and after '/' for "(1000 * U8_MAX) / rate"
* changed dev_info to dev_dbg for printing max_hw_heartbeat_ms
* added watchdog_init_timeout() for user to set timeout in DT
* switched to using devm_watchdog_register_device()
* added error message if register fails
* simplified rza_wdt_probe() return
* removed function rza_wdt_remove()
---
 drivers/watchdog/Kconfig   |   8 ++
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rza_wdt.c | 199 +
 3 files changed, 208 insertions(+)
 create mode 100644 drivers/watchdog/rza_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b5..123c516 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -701,6 +701,14 @@ config RENESAS_WDT
  This driver adds watchdog support for the integrated watchdogs in the
  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).
 
+config RENESAS_RZAWDT
+   tristate "Renesas RZ/A WDT Watchdog"
+   depends on ARCH_RENESAS || COMPILE_TEST
+   select WATCHDOG_CORE
+   help
+ This driver adds watchdog support for the integrated watchdogs in the
+ Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
+
 config ASPEED_WATCHDOG
tristate "Aspeed 2400 watchdog support"
depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c3d35e..84b897c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
 obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
+obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
 
 # AVR32 Architecture
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
new file mode 100644
index 000..e618218
--- /dev/null
+++ b/drivers/watchdog/rza_wdt.c
@@ -0,0 +1,199 @@
+/*
+ * Renesas RZ/A Series WDT Driver
+ *
+ * Copyright (C) 2017 Renesas Electronics America, Inc.
+ * Copyright (C) 2017 Chris Brandt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_TIMEOUT30
+
+/* Watchdog Timer Registers */
+#define WTCSR  0
+#define WTCSR_MAGIC0xA500
+#define WTSCR_WT   BIT(6)
+#define WTSCR_TME  BIT(5)
+#define WTSCR_CKS(i)   (i)
+
+#define WTCNT  2
+#define WTCNT_MAGIC0x5A00
+
+#define WRCSR  4
+#define WRCSR_MAGIC0x5A00
+#define WRCSR_RSTE BIT(6)
+#define WRCSR_CLEAR_WOVF   0xA500  /* special value */
+
+struct rza_wdt {
+   struct watchdog_device wdev;
+   void __iomem *base;
+   struct clk *clk;
+};
+
+static int rza_wdt_start(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /* Must dummy read WRCSR:WOVF at least once before clearing */
+   readb(priv->base + WRCSR);
+   writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+   /*
+* Start timer with slowest clock source and reset option enabled.
+*/
+   writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
+  priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_stop(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_ping(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+
+   return 0;
+}
+
+static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
+   void *data)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /* Must dummy read WRCSR:WOVF

[PATCH v6 0/3] watchdog: add wdt and reset for renesas r7s72100

2017-03-04 Thread Chris Brandt

Some Renesas SoCs do not have a reset register and the only way to do a SW
controlled reset is to use the watchdog timer. So while this series started
out by only adding a reset feature, now it's a full watchdog timer driver that
includes a reset handler.

The longest WDT overflow you can get with a RZ/A1 (R7S72100) with its 8-bit
wide counter is 125ms. Not very long.

However, by setting max_hw_heartbeat_ms, the watchdog core will now handle
pinging the WDT automatically and allow the user to set times as big as they
want. Therefore, the default timeout for this driver is 30 seconds.

Of course if system interrupts are disabled too long and wdt can't be pinged
by the watchdog core, then the system will reset before the user specified
timeout. But hey, it's better nothing.

This driver was tested on an RZ/A1 RSK board using watchdog-simple.c from
samples/watchdog/.

v6:
* rza_wdt.c changes only
* added #include 
* alphabetized #include list
* removed call to platform_set_drvdata

v5:
* rza_wdt.c changes only
* removed extra lines from file header comments
* changed (1<<#) to BIT(#)
* changed #define WTSCR_CKS(i) i to (i)
* changed format to #define SOMETHINGvalue (and align values)
* now check if clock rate < 16384
* added space before and after '/' for "(1000 * U8_MAX) / rate"
* changed dev_info to dev_dbg for printing max_hw_heartbeat_ms
* added watchdog_init_timeout() for user to set timeout in DT
* switched to using devm_watchdog_register_device()
* added error message if register fails
* simplified rza_wdt_probe() return
* removed function rza_wdt_remove()

v4:
* r7s72100.dtsi: changed from timer@ to watchdog@

v3:
* changed from a reset driver to a watchdog timer driver
* use udelay(20) instead of msleep for reset handler
* added Reviewed-by for r7s72100.dtsi and renesas-wdt.txt
* added Acked-by for renesas-wdt.txt

v2:
* added to renesas-wdt.txt instead of creating a new file
* changed "renesas,r7s72100-reset" to "renesas,r7s72100-wdt"
* changed "renesas,wdt-reset" to "renesas,rza-wdt"
* added "renesas,rza-wdt" as a fallback
* added interupt property (even though it is not used)
* added clocks property
* changed hard coded register values to defines
* added msleep to while(1) loop
* removed unnecessary #include files
* added Reviewed-by: Geert Uytterhoeven for renesas-reset.c

Chris Brandt (3):
  watchdog: add rza_wdt driver
  watchdog: renesas-wdt: add support for rza
  ARM: dts: r7s72100: Add watchdog timer

 .../devicetree/bindings/watchdog/renesas-wdt.txt   |   4 +-
 arch/arm/boot/dts/r7s72100.dtsi|   7 +
 drivers/watchdog/Kconfig   |   8 +
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rza_wdt.c | 199 +
 5 files changed, 218 insertions(+), 1 deletion(-)
 create mode 100644 drivers/watchdog/rza_wdt.c

-- 
2.10.1

[PATCH v6 3/3] ARM: dts: r7s72100: Add watchdog timer

2017-03-04 Thread Chris Brandt

Add watchdog timer support for RZ/A1.
For the RZ/A1, the only way to do a reset is to overflow the WDT, so this
is useful even if you don't need the watchdog functionality.

Signed-off-by: Chris Brandt 
Reviewed-by: Geert Uytterhoeven 
---
v4:
* changed from timer@ to watchdog@
v3:
* added Reviewed-by
v2:
* changed "renesas,r7s72100-reset" to "renesas,r7s72100-wdt"
* changed "renesas,wdt-reset" to "renesas,rza-wdt"
* added interupt property (even though it is not used)
* added clocks property
---
 arch/arm/boot/dts/r7s72100.dtsi | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index ed62e19..22f96b7 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -382,6 +382,13 @@
cache-level = <2>;
};
 
+   wdt: watchdog@fcfe {
+   compatible = "renesas,r7s72100-wdt", "renesas,rza-wdt";
+   reg = <0xfcfe 0x6>;
+   interrupts = ;
+   clocks = <_clk>;
+   };
+
i2c0: i2c@fcfee000 {
#address-cells = <1>;
#size-cells = <0>;
-- 
2.10.1

[PATCH v6 2/3] watchdog: renesas-wdt: add support for rza

2017-03-04 Thread Chris Brandt

Describe the WDT hardware in the RZ/A series.

Signed-off-by: Chris Brandt 
Reviewed-by: Geert Uytterhoeven 
Acked-by: Rob Herring 
---
v3:
* Add Acked-by, Reviewed-by.
v2:
* added to renesas-wdt.txt instead of creating a new file
* changed commit title
* added "renesas,rza-wdt" as a fallback
* added interrupts property
---
 Documentation/devicetree/bindings/watchdog/renesas-wdt.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
index da24e31..9e306af 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/renesas-wdt.txt
@@ -2,10 +2,11 @@ Renesas Watchdog Timer (WDT) Controller
 
 Required properties:
 - compatible : Should be "renesas,-wdt", and
-  "renesas,rcar-gen3-wdt" as fallback.
+  "renesas,rcar-gen3-wdt" or "renesas,rza-wdt" as fallback.
   Examples with soctypes are:
 - "renesas,r8a7795-wdt" (R-Car H3)
 - "renesas,r8a7796-wdt" (R-Car M3-W)
+- "renesas,r7s72100-wdt" (RZ/A1)
 
   When compatible with the generic version, nodes must list the SoC-specific
   version corresponding to the platform first, followed by the generic
@@ -17,6 +18,7 @@ Required properties:
 Optional properties:
 - timeout-sec : Contains the watchdog timeout in seconds
 - power-domains : the power domain the WDT belongs to
+- interrupts: Some WDTs have an interrupt when used in interval timer mode
 
 Examples:
 
-- 
2.10.1

RE: [PATCH v5 1/3] watchdog: add rza_wdt driver

2017-03-04 Thread Chris Brandt

On Saturday, March 04, 2017, Guenter Roeck wrote:
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> 
> Also needs to include . While at it, please order include
> files alphabetically.

OK.

> > +   platform_set_drvdata(pdev, priv);
> 
> This is now unnecessary.

OK.


Thank you,
Chris

Re: [PATCH v5 1/3] watchdog: add rza_wdt driver

2017-03-04 Thread Guenter Roeck


On 03/03/2017 09:31 AM, Chris Brandt wrote:

Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset
handler is also included since a WDT overflow is the only method for
restarting an RZ/A SoC.

Signed-off-by: Chris Brandt 
---
v2:
* removed extra lines from file header comments
* changed (1<<#) to BIT(#)
* changed #define WTSCR_CKS(i) i to (i)
* changed format to #define SOMETHINGvalue (and align values)
* now check if clock rate < 16384
* added space before and after '/' for "(1000 * U8_MAX) / rate"
* changed dev_info to dev_dbg for printing max_hw_heartbeat_ms
* added watchdog_init_timeout() for user to set timeout in DT
* switched to using devm_watchdog_register_device()
* added error message if register fails
* simplified rza_wdt_probe() return
* removed function rza_wdt_remove()
---
 drivers/watchdog/Kconfig   |   8 ++
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rza_wdt.c | 199 +
 3 files changed, 208 insertions(+)
 create mode 100644 drivers/watchdog/rza_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b5..123c516 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -701,6 +701,14 @@ config RENESAS_WDT
  This driver adds watchdog support for the integrated watchdogs in the
  Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT).

+config RENESAS_RZAWDT
+   tristate "Renesas RZ/A WDT Watchdog"
+   depends on ARCH_RENESAS || COMPILE_TEST
+   select WATCHDOG_CORE
+   help
+ This driver adds watchdog support for the integrated watchdogs in the
+ Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
+
 config ASPEED_WATCHDOG
tristate "Aspeed 2400 watchdog support"
depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c3d35e..84b897c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
 obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
+obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o
 obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o

 # AVR32 Architecture
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
new file mode 100644
index 000..76a9c1f
--- /dev/null
+++ b/drivers/watchdog/rza_wdt.c
@@ -0,0 +1,199 @@
+/*
+ * Renesas RZ/A Series WDT Driver
+ *
+ * Copyright (C) 2017 Renesas Electronics America, Inc.
+ * Copyright (C) 2017 Chris Brandt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+


Also needs to include . While at it, please order include
files alphabetically.


+#define DEFAULT_TIMEOUT30
+
+/* Watchdog Timer Registers */
+#define WTCSR  0
+#define WTCSR_MAGIC0xA500
+#define WTSCR_WT   BIT(6)
+#define WTSCR_TME  BIT(5)
+#define WTSCR_CKS(i)   (i)
+
+#define WTCNT  2
+#define WTCNT_MAGIC0x5A00
+
+#define WRCSR  4
+#define WRCSR_MAGIC0x5A00
+#define WRCSR_RSTE BIT(6)
+#define WRCSR_CLEAR_WOVF   0xA500  /* special value */
+
+struct rza_wdt {
+   struct watchdog_device wdev;
+   void __iomem *base;
+   struct clk *clk;
+};
+
+static int rza_wdt_start(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /* Must dummy read WRCSR:WOVF at least once before clearing */
+   readb(priv->base + WRCSR);
+   writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
+
+   /*
+* Start timer with slowest clock source and reset option enabled.
+*/
+   writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+   writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
+  priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_stop(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   return 0;
+}
+
+static int rza_wdt_ping(struct watchdog_device *wdev)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+
+   return 0;
+}
+
+static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action,
+   void *data)
+{
+   struct rza_wdt *priv = watchdog_get_drvdata(wdev);
+
+   /* Stop timer */
+   writew(WTCSR_MAGIC | 0, priv->base + WTCSR);
+
+   /*

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

2017-03-04 Thread Sakari Ailus

Hi Hans (and Laurent).

On Sat, Mar 04, 2017 at 11:53:45AM +0100, Hans Verkuil wrote:
> Hi Laurent,
> 
> Here is my review:
> 
> On 28/02/17 16:03, Laurent Pinchart wrote:
> >V4L2 exposes parameters that influence buffers sizes through the format
> >ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
> 
> S_SELECTION should be mentioned here as well (more about that later).
> 
> >not part of the format structure may also influence buffer sizes or
> >buffer layout in general. One existing such parameter is rotation, which
> >is implemented by the VIDIOC_ROTATE control and thus exposed through the
> >V4L2 control ioctls.
> >
> >The interaction between those parameters and buffers is currently only
> >partially specified by the V4L2 API. In particular interactions between
> >controls and buffers isn't specified at all. The behaviour of the
> >VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
> >specified.
> >
> >This commit clearly defines and documents the interactions between
> >formats, controls and buffers.
> >
> >The preparatory discussions for the documentation change considered
> >completely disallowing controls that change the buffer size or layout,
> >in favour of extending the format API with a new ioctl that would bundle
> >those controls with format information. The idea has been rejected, as
> >this would essentially be a restricted version of the upcoming request
> >API that wouldn't bring any additional value.
> >
> >Another option we have considered was to mandate the use of the request
> >API to modify controls that influence buffer size or layout. This has
> >also been rejected on the grounds that requiring the request API to
> >change rotation even when streaming is stopped would significantly
> >complicate implementation of drivers and usage of the V4L2 API for
> >applications.
> >
> >Applications will however be required to use the upcoming request API to
> >change at runtime formats or controls that influence the buffer size or
> >layout, because of the need to synchronize buffers with the formats and
> >controls. Otherwise there would be no way to interpret the content of a
> >buffer correctly.
> >
> >Signed-off-by: Laurent Pinchart 
> >---
> > Documentation/media/uapi/v4l/buffer.rst | 88 
> > +
> > 1 file changed, 88 insertions(+)
> >
> >diff --git a/Documentation/media/uapi/v4l/buffer.rst 
> >b/Documentation/media/uapi/v4l/buffer.rst
> >index ac58966ccb9b..5c58db98ab7a 100644
> >--- a/Documentation/media/uapi/v4l/buffer.rst
> >+++ b/Documentation/media/uapi/v4l/buffer.rst
> >@@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the 
> >CAPTURE video
> > buffer.
> >
> >
> >+Interactions between formats, controls and buffers
> >+==
> >+
> >+V4L2 exposes parameters that influence the buffer size, or the way data is
> >+laid out in the buffer. Those parameters are exposed through both formats 
> >and
> >+controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
> >+that modifies the direction in which pixels are stored in the buffer, as 
> >well
> >+as the buffer size when the selected format includes padding at the end of
> >+lines.
> >+
> >+The set of information needed to interpret the content of a buffer (e.g. the
> >+pixel format, the line stride, the tiling orientation or the rotation) is
> >+collectively referred to in the rest of this section as the buffer layout.
> >+
> >+Modifying formats or controls that influence the buffer size or layout 
> >require
> >+the stream to be stopped. Any attempt at such a modification while the 
> >stream
> >+is active shall cause the format or control set ioctl to return the 
> >``EBUSY``
> >+error code.
> 
> This is not what happens today: it's not the streaming part that causes EBUSY 
> to
> be returned but whether or not buffers are allocated.
> 
> Today we do not support changing buffer sizes on the fly, so any attempt to
> call an ioctl that would change the buffer size is blocked and EBUSY is 
> returned.
> To be precise: drivers call vb2_is_busy() to determine this.

It certainly shouldn't be like that. Not allowing S_FMT() while there are
buffers allocated makes CREATE_BUFS entirely useless.

> 
> To my knowledge all vb2-using drivers behave like this. There may be old 
> drivers
> that do not do this (and these have a high likelyhood of being wrong).

What's really needed is that the driver verifies that the buffer is large
enough to be used for a given format. vb2_is_busy() shouldn't be used to
check whether setting format is allowed.

> 
> >+
> >+Controls that only influence the buffer layout can be modified at any time
> >+when the stream is stopped. As they don't influence the buffer size, no
> >+special handling is needed to synchronize those controls with buffer
> >+allocation.
> >+
> >+Formats and controls that influence the buffer size interact

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

2017-03-04 Thread Sakari Ailus

Hi Hans,

On Sat, Mar 04, 2017 at 11:57:32AM +0100, Hans Verkuil wrote:
...
> >>+To simplify their implementation, drivers may also require buffers to be
> >>+reallocated in order to change formats or controls that influence the 
> >>buffer
> >>+size. In that case, to perform such changes, userspace applications shall
> >>+first stop the video stream with the :c:func:`VIDIOC_STREAMOFF` ioctl if it
> >>+is running and free all buffers with the :c:func:`VIDIOC_REQBUFS` ioctl if
> >>+they are allocated. The format or controls can then be modified, and 
> >>buffers
> >>+shall then be reallocated and the stream restarted. A typical ioctl 
> >>sequence
> >>+is
> >>+
> >>+ #. VIDIOC_STREAMOFF
> >>+ #. VIDIOC_REQBUFS(0)
> >>+ #. VIDIOC_S_FMT
> >>+ #. VIDIOC_S_EXT_CTRLS
> >
> >Same here.
> >
> >Would it be safe to say that controls are changed first? I wonder if there
> >could be special cases where this wouldn't apply though. It could ultimately
> >come down to hardware features: rotation might be only available for certain
> >formats so you'd need to change the format first to enable rotation.
> >
> >What you're documenting above is a typical sequence so it doesn't have to be
> >applicable to all potential hardware. I might mention there could be such
> >dependencies. I wonder if one exists at the moment. No?
> 
> The way V4L2 works is that the last ioctl called gets 'preference'. So the
> driver should attempt to satisfy the ioctl, even if that means undoing 
> previous
> ioctls. In other words, V4L2 allows any order, but the end-result might be
> different depending on the hardware capabilities.

Indeed. But the above sequence suggests that formats are set before
controls. I suggested to clarify that part.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

[PATCH] drm: rcar-du: Arm the page flip event after queuing the page flip.

2017-03-04 Thread Laurent Pinchart

The page flip event is armed in the atomic begin handler, creating a
race condition with the frame end interrupt that could send the event
before the atomic operation actually completes. To avoid that, arm the
event in the atomic flush handler after queuing the page flip.

This change doesn't fully close the race window, as the frame end
interrupt could be generated before the page flip is committed to
hardware but only handled after the event is armed. However, the race
window is now much smaller.

Signed-off-by: Laurent Pinchart 
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 22 +++---
 1 file changed, 11 insertions(+), 11 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..7f6663ccd797 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -502,17 +502,6 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc 
*crtc,
  struct drm_crtc_state *old_crtc_state)
 {
struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
-   struct drm_device *dev = rcrtc->crtc.dev;
-   unsigned long flags;
-
-   if (crtc->state->event) {
-   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-
-   spin_lock_irqsave(>event_lock, flags);
-   rcrtc->event = crtc->state->event;
-   crtc->state->event = NULL;
-   spin_unlock_irqrestore(>event_lock, flags);
-   }
 
if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
rcar_du_vsp_atomic_begin(rcrtc);
@@ -522,11 +511,22 @@ static void rcar_du_crtc_atomic_flush(struct drm_crtc 
*crtc,
  struct drm_crtc_state *old_crtc_state)
 {
struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+   struct drm_device *dev = rcrtc->crtc.dev;
+   unsigned long flags;
 
rcar_du_crtc_update_planes(rcrtc);
 
if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
rcar_du_vsp_atomic_flush(rcrtc);
+
+   if (crtc->state->event) {
+   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+
+   spin_lock_irqsave(>event_lock, flags);
+   rcrtc->event = crtc->state->event;
+   crtc->state->event = NULL;
+   spin_unlock_irqrestore(>event_lock, flags);
+   }
 }
 
 static const struct drm_crtc_helper_funcs crtc_helper_funcs = {
-- 
Regards,

Laurent Pinchart

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

2017-03-04 Thread Laurent Pinchart

Hi Kieran,

Thank you for the patch.

On Saturday 04 Mar 2017 02:01:19 Kieran Bingham wrote:
> Currently we process page flip events on every display interrupt,
> however this does not take into consideration the processing time needed
> by the VSP1 utilised in the pipeline.
> 
> Register a callback with the VSP driver to obtain completion events, and
> track them so that we only perform page flips when the full display
> pipeline has completed for the frame.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> v2:
>  - Commit message completely re-worded for patch re-work.
>  - drm_crtc_handle_vblank() re-instated in event of rcrtc->pending
>  - removed passing of unnecessary 'data' through callbacks
>  - perform page flips from the VSP completion handler
>  - add locking around pending flags
> 
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 +++--
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 29 +++-
>  3 files changed, 39 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..b7ff00bb45de
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -299,7 +299,7 @@ static void rcar_du_crtc_update_planes(struct
> rcar_du_crtc *rcrtc) * Page Flip
>   */
> 
> -static void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
> +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
>  {
>   struct drm_pending_vblank_event *event;
>   struct drm_device *dev = rcrtc->crtc.dev;
> @@ -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);

No need for parenthesis.

>   spin_unlock_irqrestore(>event_lock, flags);
> 
>   return pending;
> @@ -579,6 +579,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
> 
>   if (status & DSSR_FRM) {
>   drm_crtc_handle_vblank(>crtc);
> +
> + if (rcrtc->pending) {
> + trace_printk("VBlank loss due to VSP Overrun\n");
> + return IRQ_HANDLED;
> + }
> +

More than that, now that the VSP completion handler finishes the page flip, 
you should skip the rcar_du_crtc_finish_page_flip() call here unconditionally 
on Gen3.

Something like

struct rcar_du_crtc *rcrtc = arg;
struct rcar_du_device *rcdu = rcrtc->group->dev;
...

if (status & DSSR_FRM) {
drm_crtc_handle_vblank(>crtc);

if (rcdu->info->gen < 3)
rcar_du_crtc_finish_page_flip(rcrtc);

ret = IRQ_HANDLED;
}

>   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..b73ec6de7af4
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -47,6 +47,7 @@ struct rcar_du_crtc {
> 
>   struct drm_pending_vblank_event *event;
>   wait_queue_head_t flip_wait;
> + bool pending;
> 
>   unsigned int outputs;
> 
> @@ -71,5 +72,6 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
> 
>  void rcar_du_crtc_route_output(struct drm_crtc *crtc,
>  enum rcar_du_output output);
> +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
> 
>  #endif /* __RCAR_DU_CRTC_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b0ff304ce3dc..1fcd311badb1
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -28,6 +28,22 @@
>  #include "rcar_du_kms.h"
>  #include "rcar_du_vsp.h"
> 
> +static void rcar_du_vsp_complete(void *private)
> +{
> + struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
> + struct drm_device *dev = crtc->crtc.dev;
> + unsigned long flags;
> + bool pending;
> +
> + spin_lock_irqsave(>event_lock, flags);
> + pending = crtc->pending;
> + crtc->pending = false;
> + spin_unlock_irqrestore(>event_lock, flags);
> +
> + if (pending)
> + rcar_du_crtc_finish_page_flip(crtc);

This seems to duplicate the synchronization mechanism based on events in 
rcar_du_crtc_atomic_begin(). I need to check that in more details.

> +}
> +
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  {
>   const struct drm_display_mode *mode = >crtc.state-
>adjusted_mode;
> @@ -35,6 +51,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>   struct vsp1_du_lif_config cfg = {
>   .width = mode->hdisplay,
>   .height = mode->vdisplay,
> +

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

2017-03-04 Thread Hans Verkuil


On 02/03/17 16:37, Sakari Ailus wrote:

Hi Laurent,

On Tue, Feb 28, 2017 at 05:03:19PM +0200, Laurent Pinchart wrote:

V4L2 exposes parameters that influence buffers sizes through the format
ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters
not part of the format structure may also influence buffer sizes or
buffer layout in general. One existing such parameter is rotation, which
is implemented by the VIDIOC_ROTATE control and thus exposed through the
V4L2 control ioctls.

The interaction between those parameters and buffers is currently only
partially specified by the V4L2 API. In particular interactions between
controls and buffers isn't specified at all. The behaviour of the
VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
specified.

This commit clearly defines and documents the interactions between
formats, controls and buffers.

The preparatory discussions for the documentation change considered
completely disallowing controls that change the buffer size or layout,
in favour of extending the format API with a new ioctl that would bundle
those controls with format information. The idea has been rejected, as
this would essentially be a restricted version of the upcoming request
API that wouldn't bring any additional value.

Another option we have considered was to mandate the use of the request
API to modify controls that influence buffer size or layout. This has
also been rejected on the grounds that requiring the request API to
change rotation even when streaming is stopped would significantly
complicate implementation of drivers and usage of the V4L2 API for
applications.

Applications will however be required to use the upcoming request API to
change at runtime formats or controls that influence the buffer size or
layout, because of the need to synchronize buffers with the formats and
controls. Otherwise there would be no way to interpret the content of a
buffer correctly.

Signed-off-by: Laurent Pinchart 
---
 Documentation/media/uapi/v4l/buffer.rst | 88 +
 1 file changed, 88 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst 
b/Documentation/media/uapi/v4l/buffer.rst
index ac58966ccb9b..5c58db98ab7a 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the CAPTURE 
video
 buffer.


+Interactions between formats, controls and buffers
+==
+
+V4L2 exposes parameters that influence the buffer size, or the way data is
+laid out in the buffer. Those parameters are exposed through both formats and
+controls. One example of such a control is the ``V4L2_CI, and also documented 
in the documentation of

those ioctls (if that isn't done already). The latter can be done later, or 
D_ROTATE`` control

+that modifies the direction in which pixels are stored in the buffer, as well
+as the buffer size when the selected format includes padding at the end of
+lines.
+
+The set of information needed to interpret the content of a buffer (e.g. the
+pixel format, the line stride, the tiling orientation or the rotation) is
+collectively referred to in the rest of this section as the buffer layout.
+
+Modifying formats or controls that influence the buffer size or layout require
+the stream to be stopped. Any attempt at such a modification while the stream
+is active shall cause the format or control set ioctl to return the ``EBUSY``
+error code.
+
+Controls that only influence the buffer layout can be modified at any time
+when the stream is stopped. As they don't influence the buffer size, no
+special handling is needed to synchronize those controls with buffer
+allocation.
+
+Formats and controls that influence the buffer size interact with buffer
+allocation. As buffer allocation is an expensive operation, drivers should
+allow format or controls that influence the buffer size to be changed with
+buffers allocated. A typical ioctl sequence to modify format and controls is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_S_FMT
+ #. VIDIOC_S_EXT_CTRLS


Which one do you set first, the format or the controls? Supposedly the user
would have to get the format again after setting the ROTATE control.


+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+Queued buffers must be large enough for the new format or controls.
+
+Drivers shall return a ``ENOSPC`` error in response to format change
+(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
+:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
+currently queued. As a simplification, drivers are allowed to return an error
+from these ioctls if any buffer is currently queued, without checking the
+queued buffers sizes. Drivers shall also return a ``ENOSPC`` error from the
+:c:func:`VIDIOC_QBUF` ioctl if the buffer being queued is too small for the
+current format or

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

2017-03-04 Thread Hans Verkuil


Hi Laurent,

Here is my review:

On 28/02/17 16:03, Laurent Pinchart wrote:

V4L2 exposes parameters that influence buffers sizes through the format
ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT and VIDIO_S_FMT). Other parameters


S_SELECTION should be mentioned here as well (more about that later).


not part of the format structure may also influence buffer sizes or
buffer layout in general. One existing such parameter is rotation, which
is implemented by the VIDIOC_ROTATE control and thus exposed through the
V4L2 control ioctls.

The interaction between those parameters and buffers is currently only
partially specified by the V4L2 API. In particular interactions between
controls and buffers isn't specified at all. The behaviour of the
VIDIOC_S_FMT ioctl when buffers are allocated is also not fully
specified.

This commit clearly defines and documents the interactions between
formats, controls and buffers.

The preparatory discussions for the documentation change considered
completely disallowing controls that change the buffer size or layout,
in favour of extending the format API with a new ioctl that would bundle
those controls with format information. The idea has been rejected, as
this would essentially be a restricted version of the upcoming request
API that wouldn't bring any additional value.

Another option we have considered was to mandate the use of the request
API to modify controls that influence buffer size or layout. This has
also been rejected on the grounds that requiring the request API to
change rotation even when streaming is stopped would significantly
complicate implementation of drivers and usage of the V4L2 API for
applications.

Applications will however be required to use the upcoming request API to
change at runtime formats or controls that influence the buffer size or
layout, because of the need to synchronize buffers with the formats and
controls. Otherwise there would be no way to interpret the content of a
buffer correctly.

Signed-off-by: Laurent Pinchart 
---
 Documentation/media/uapi/v4l/buffer.rst | 88 +
 1 file changed, 88 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst 
b/Documentation/media/uapi/v4l/buffer.rst
index ac58966ccb9b..5c58db98ab7a 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -34,6 +34,94 @@ flags are copied from the OUTPUT video buffer to the CAPTURE 
video
 buffer.


+Interactions between formats, controls and buffers
+==
+
+V4L2 exposes parameters that influence the buffer size, or the way data is
+laid out in the buffer. Those parameters are exposed through both formats and
+controls. One example of such a control is the ``V4L2_CID_ROTATE`` control
+that modifies the direction in which pixels are stored in the buffer, as well
+as the buffer size when the selected format includes padding at the end of
+lines.
+
+The set of information needed to interpret the content of a buffer (e.g. the
+pixel format, the line stride, the tiling orientation or the rotation) is
+collectively referred to in the rest of this section as the buffer layout.
+
+Modifying formats or controls that influence the buffer size or layout require
+the stream to be stopped. Any attempt at such a modification while the stream
+is active shall cause the format or control set ioctl to return the ``EBUSY``
+error code.


This is not what happens today: it's not the streaming part that causes EBUSY to
be returned but whether or not buffers are allocated.

Today we do not support changing buffer sizes on the fly, so any attempt to
call an ioctl that would change the buffer size is blocked and EBUSY is 
returned.
To be precise: drivers call vb2_is_busy() to determine this.

To my knowledge all vb2-using drivers behave like this. There may be old drivers
that do not do this (and these have a high likelyhood of being wrong).


+
+Controls that only influence the buffer layout can be modified at any time
+when the stream is stopped. As they don't influence the buffer size, no
+special handling is needed to synchronize those controls with buffer
+allocation.
+
+Formats and controls that influence the buffer size interact with buffer
+allocation. As buffer allocation is an expensive operation, drivers should
+allow format or controls that influence the buffer size to be changed with
+buffers allocated. A typical ioctl sequence to modify format and controls is
+
+ #. VIDIOC_STREAMOFF
+ #. VIDIOC_S_FMT
+ #. VIDIOC_S_EXT_CTRLS
+ #. VIDIOC_QBUF
+ #. VIDIOC_STREAMON
+
+Queued buffers must be large enough for the new format or controls.
+
+Drivers shall return a ``ENOSPC`` error in response to format change
+(:c:func:`VIDIOC_S_FMT`) or control changes (:c:func:`VIDIOC_S_CTRL` or
+:c:func:`VIDIOC_S_EXT_CTRLS`) if buffers too small for the new format are
+currently queued. As a simplification, drivers are allowed

[PATCH v2] drm: rcar-du: Arm the page flip event after queuing the page flip

Re: [PATCH v2 2/3] v4l: vsp1: Postpone page flip in event of display list race

[PATCH] drm: rcar-du: Remove wait field from rcar_du_device structure

Re: [PATCH v2 1/3] v4l: vsp1: extend VSP1 module API to allow DRM callbacks

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

[PATCH v6 1/3] watchdog: add rza_wdt driver

[PATCH v6 0/3] watchdog: add wdt and reset for renesas r7s72100

[PATCH v6 3/3] ARM: dts: r7s72100: Add watchdog timer

[PATCH v6 2/3] watchdog: renesas-wdt: add support for rza

RE: [PATCH v5 1/3] watchdog: add rza_wdt driver

Re: [PATCH v5 1/3] watchdog: add rza_wdt driver

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

[PATCH] drm: rcar-du: Arm the page flip event after queuing the page flip.

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

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

Re: [PATCH v2 2/3] v4l: Clearly document interactions between formats, controls and buffers

17 matches

Site Navigation

Mail list logo

Footer information