Re: [PATCH v2 11/15] v4l: vsp1: Add per-display list internal completion notification support

[Kieran Bingham]

Hi Laurent,

On 05/04/18 10:18, Laurent Pinchart wrote:
> Display list completion is already reported to the frame end handler,
> but that mechanism is global to all display lists. In order to implement
> BRU and BRS reassignment in DRM pipelines we will need to commit a
> display list and wait for its completion internally, without reporting
> it to the DRM driver. Extend the display list API to support such an
> internal use of the display list.
> 
> Signed-off-by: Laurent Pinchart 
> ---
> Changes since v1:
> 
> - Use the frame end status flags to report notification
> - Rename the notify flag to internal

This reads much better to me.

Thanks for the respin.

Reviewed-by: Kieran Bingham 


> ---
>  drivers/media/platform/vsp1/vsp1_dl.c| 23 ++-
>  drivers/media/platform/vsp1/vsp1_dl.h|  3 ++-
>  drivers/media/platform/vsp1/vsp1_drm.c   |  2 +-
>  drivers/media/platform/vsp1/vsp1_video.c |  2 +-
>  4 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index 662fa2a347c9..30ad491605ff 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -72,6 +72,7 @@ struct vsp1_dl_body {
>   * @fragments: list of extra display list bodies
>   * @has_chain: if true, indicates that there's a partition chain
>   * @chain: entry in the display list partition chain
> + * @internal: whether the display list is used for internal purpose
>   */
>  struct vsp1_dl_list {
>   struct list_head list;
> @@ -85,6 +86,8 @@ struct vsp1_dl_list {
>  
>   bool has_chain;
>   struct list_head chain;
> +
> + bool internal;
>  };
>  
>  enum vsp1_dl_mode {
> @@ -550,8 +553,16 @@ static void vsp1_dl_list_commit_continuous(struct 
> vsp1_dl_list *dl)
>* case we can't replace the queued list by the new one, as we could
>* race with the hardware. We thus mark the update as pending, it will
>* be queued up to the hardware by the frame end interrupt handler.
> +  *
> +  * If a display list is already pending we simply drop it as the new
> +  * display list is assumed to contain a more recent configuration. It is
> +  * an error if the already pending list has the internal flag set, as
> +  * there is then a process waiting for that list to complete. This
> +  * shouldn't happen as the waiting process should perform proper
> +  * locking, but warn just in case.
>*/
>   if (vsp1_dl_list_hw_update_pending(dlm)) {
> + WARN_ON(dlm->pending && dlm->pending->internal);
>   __vsp1_dl_list_put(dlm->pending);
>   dlm->pending = dl;
>   return;
> @@ -581,7 +592,7 @@ static void vsp1_dl_list_commit_singleshot(struct 
> vsp1_dl_list *dl)
>   dlm->active = dl;
>  }
>  
> -void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
> +void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
>  {
>   struct vsp1_dl_manager *dlm = dl->dlm;
>   struct vsp1_dl_list *dl_child;
> @@ -598,6 +609,8 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   }
>   }
>  
> + dl->internal = internal;
> +
>   spin_lock_irqsave(>lock, flags);
>  
>   if (dlm->singleshot)
> @@ -624,6 +637,10 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   * raced with the frame end interrupt. The function always returns with the 
> flag
>   * set in header mode as display list processing is then not continuous and
>   * races never occur.
> + *
> + * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the previous display 
> list
> + * has completed and had been queued with the internal notification flag.
> + * Internal notification is only supported for continuous mode.
>   */
>  unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
> @@ -656,6 +673,10 @@ unsigned int vsp1_dlm_irq_frame_end(struct 
> vsp1_dl_manager *dlm)
>* frame end interrupt. The display list thus becomes active.
>*/
>   if (dlm->queued) {
> + if (dlm->queued->internal)
> + flags |= VSP1_DL_FRAME_END_INTERNAL;
> + dlm->queued->internal = false;
> +
>   __vsp1_dl_list_put(dlm->active);
>   dlm->active = dlm->queued;
>   dlm->queued = NULL;
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h 
> b/drivers/media/platform/vsp1/vsp1_dl.h
> index cbc2fc53e10b..1a5bbd5ddb7b 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -21,6 +21,7 @@ struct vsp1_dl_list;
>  struct vsp1_dl_manager;
>  
>  #define VSP1_DL_FRAME_END_COMPLETED  BIT(0)
> +#define VSP1_DL_FRAME_END_INTERNAL   BIT(1)
>  
>  void vsp1_dlm_setup(struct vsp1_device *vsp1);
>  
> @@ -34,7 +35,7 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager 
> 

[PATCH v2 11/15] v4l: vsp1: Add per-display list internal completion notification support

[Laurent Pinchart]

Display list completion is already reported to the frame end handler,
but that mechanism is global to all display lists. In order to implement
BRU and BRS reassignment in DRM pipelines we will need to commit a
display list and wait for its completion internally, without reporting
it to the DRM driver. Extend the display list API to support such an
internal use of the display list.

Signed-off-by: Laurent Pinchart 
---
Changes since v1:

- Use the frame end status flags to report notification
- Rename the notify flag to internal
---
 drivers/media/platform/vsp1/vsp1_dl.c| 23 ++-
 drivers/media/platform/vsp1/vsp1_dl.h|  3 ++-
 drivers/media/platform/vsp1/vsp1_drm.c   |  2 +-
 drivers/media/platform/vsp1/vsp1_video.c |  2 +-
 4 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index 662fa2a347c9..30ad491605ff 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -72,6 +72,7 @@ struct vsp1_dl_body {
  * @fragments: list of extra display list bodies
  * @has_chain: if true, indicates that there's a partition chain
  * @chain: entry in the display list partition chain
+ * @internal: whether the display list is used for internal purpose
  */
 struct vsp1_dl_list {
struct list_head list;
@@ -85,6 +86,8 @@ struct vsp1_dl_list {
 
bool has_chain;
struct list_head chain;
+
+   bool internal;
 };
 
 enum vsp1_dl_mode {
@@ -550,8 +553,16 @@ static void vsp1_dl_list_commit_continuous(struct 
vsp1_dl_list *dl)
 * case we can't replace the queued list by the new one, as we could
 * race with the hardware. We thus mark the update as pending, it will
 * be queued up to the hardware by the frame end interrupt handler.
+*
+* If a display list is already pending we simply drop it as the new
+* display list is assumed to contain a more recent configuration. It is
+* an error if the already pending list has the internal flag set, as
+* there is then a process waiting for that list to complete. This
+* shouldn't happen as the waiting process should perform proper
+* locking, but warn just in case.
 */
if (vsp1_dl_list_hw_update_pending(dlm)) {
+   WARN_ON(dlm->pending && dlm->pending->internal);
__vsp1_dl_list_put(dlm->pending);
dlm->pending = dl;
return;
@@ -581,7 +592,7 @@ static void vsp1_dl_list_commit_singleshot(struct 
vsp1_dl_list *dl)
dlm->active = dl;
 }
 
-void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
+void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
 {
struct vsp1_dl_manager *dlm = dl->dlm;
struct vsp1_dl_list *dl_child;
@@ -598,6 +609,8 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
}
}
 
+   dl->internal = internal;
+
spin_lock_irqsave(>lock, flags);
 
if (dlm->singleshot)
@@ -624,6 +637,10 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
  * raced with the frame end interrupt. The function always returns with the 
flag
  * set in header mode as display list processing is then not continuous and
  * races never occur.
+ *
+ * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the previous display list
+ * has completed and had been queued with the internal notification flag.
+ * Internal notification is only supported for continuous mode.
  */
 unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 {
@@ -656,6 +673,10 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager 
*dlm)
 * frame end interrupt. The display list thus becomes active.
 */
if (dlm->queued) {
+   if (dlm->queued->internal)
+   flags |= VSP1_DL_FRAME_END_INTERNAL;
+   dlm->queued->internal = false;
+
__vsp1_dl_list_put(dlm->active);
dlm->active = dlm->queued;
dlm->queued = NULL;
diff --git a/drivers/media/platform/vsp1/vsp1_dl.h 
b/drivers/media/platform/vsp1/vsp1_dl.h
index cbc2fc53e10b..1a5bbd5ddb7b 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.h
+++ b/drivers/media/platform/vsp1/vsp1_dl.h
@@ -21,6 +21,7 @@ struct vsp1_dl_list;
 struct vsp1_dl_manager;
 
 #define VSP1_DL_FRAME_END_COMPLETEDBIT(0)
+#define VSP1_DL_FRAME_END_INTERNAL BIT(1)
 
 void vsp1_dlm_setup(struct vsp1_device *vsp1);
 
@@ -34,7 +35,7 @@ unsigned 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);
 void vsp1_dl_list_write(struct vsp1_dl_list *dl, u32 reg, u32 data);
-void vsp1_dl_list_commit(struct vsp1_dl_list *dl);
+void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal);
 
 struct vsp1_dl_body