Re: [PATCH v11 10/10] media: vsp1: Move video configuration to a cached dlb

2018-05-18 Thread Laurent Pinchart
Hi Kieran,

Thank you for the patch.

On Friday, 18 May 2018 23:42:03 EEST Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> We are now able to configure a pipeline directly into a local display
> list body. Take advantage of this fact, and create a cacheable body to
> store the configuration of the pipeline in the video object.

s/video object/pipeline object/

> 
> vsp1_video_pipeline_run() is now the last user of the pipe->dl object.
> Convert this function to use the cached video->config body and obtain a

s/video->config/pipe->stream_config/

> local display list reference.
> 
> Attach the video->stream_config body to the display list when needed

s/video/pipe/

> before committing to hardware.
> 
> Use a flag 'configured' to know when we should attach our stream_config
> to the next outgoing display list to reconfigure the hardware in the
> event of our first frame, or the first frame following a suspend/resume
> cycle.
> 
> Our video DL usage now looks like the below output:
> 
> dl->body0 contains our disposable runtime configuration. Max 41.
> dl_child->body0 is our partition specific configuration. Max 12.
> dl->bodies shows our constant configuration and LUTs.
> 
>   These two are LUT/CLU:
>  * dl->bodies[x]->num_entries 256 / max 256
>  * dl->bodies[x]->num_entries 4914 / max 4914
> 
> Which shows that our 'constant' configuration cache is currently
> utilised to a maximum of 64 entries.
> 
> trace-cmd report | \
> grep max | sed 's/.*vsp1_dl_list_commit://g' | sort | uniq;
> 
>   dl->body0->num_entries 13 / max 128
>   dl->body0->num_entries 14 / max 128
>   dl->body0->num_entries 16 / max 128
>   dl->body0->num_entries 20 / max 128
>   dl->body0->num_entries 27 / max 128
>   dl->body0->num_entries 34 / max 128
>   dl->body0->num_entries 41 / max 128
>   dl_child->body0->num_entries 10 / max 128
>   dl_child->body0->num_entries 12 / max 128
>   dl->bodies[x]->num_entries 15 / max 128
>   dl->bodies[x]->num_entries 16 / max 128
>   dl->bodies[x]->num_entries 17 / max 128
>   dl->bodies[x]->num_entries 18 / max 128
>   dl->bodies[x]->num_entries 20 / max 128
>   dl->bodies[x]->num_entries 21 / max 128
>   dl->bodies[x]->num_entries 256 / max 256
>   dl->bodies[x]->num_entries 31 / max 128
>   dl->bodies[x]->num_entries 32 / max 128
>   dl->bodies[x]->num_entries 39 / max 128
>   dl->bodies[x]->num_entries 40 / max 128
>   dl->bodies[x]->num_entries 47 / max 128
>   dl->bodies[x]->num_entries 48 / max 128
>   dl->bodies[x]->num_entries 4914 / max 4914
>   dl->bodies[x]->num_entries 55 / max 128
>   dl->bodies[x]->num_entries 56 / max 128
>   dl->bodies[x]->num_entries 63 / max 128
>   dl->bodies[x]->num_entries 64 / max 128
> 
> Signed-off-by: Kieran Bingham 
> ---
> v11:
>  - Remove dlbs pool from video object.
>Utilise the DLM pool for video->stream_config
>  - Improve comments
>  - clear the video->stream_config after it is released
>object.
>  - stream_config and configured flag return to the pipe object.
> 
> v10:
>  - Removed pipe->configured flag, and use
>pipe->state == VSP1_PIPELINE_STOPPED instead.
> 
> v8:
>  - Fix comments
>  - Rename video->pipe_config -> video->stream_config
> 
> v4:
>  - Adjust pipe configured flag to be reset on resume rather than suspend
>  - rename dl_child, dl_next
> 
> v3:
>  - 's/fragment/body/', 's/fragments/bodies/'
>  - video dlb cache allocation increased from 2 to 3 dlbs
> 
>  drivers/media/platform/vsp1/vsp1_dl.c| 10 +++-
>  drivers/media/platform/vsp1/vsp1_dl.h|  1 +-
>  drivers/media/platform/vsp1/vsp1_pipe.h  |  6 +-
>  drivers/media/platform/vsp1/vsp1_video.c | 69 +++--
>  4 files changed, 56 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index c7fa1cb088cd..0f97305de965
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -813,6 +813,11 @@ void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
>   dlm->pending = NULL;
>  }
> 
> +struct vsp1_dl_body *vsp1_dlm_dl_body_get(struct vsp1_dl_manager *dlm)
> +{
> + return vsp1_dl_body_get(dlm->pool);
> +}
> +
>  struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
>   unsigned int index,
>   unsigned int prealloc)
> @@ -838,13 +843,14 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct
> vsp1_device *vsp1,
>* Initialize the display list body and allocate DMA memory for the body
>* and the optional header. Both are allocated together to avoid memory
>* fragmentation, with the header located right after the body in
> -  * memory.
> +  * memory. An extra body is allocated on top of the prealloc to account
> +  * for the cached body used by the vsp1_video object.

s/vsp1_video/vsp1_pipeline/

Apart from that,

Reviewed-by: 

[PATCH v11 10/10] media: vsp1: Move video configuration to a cached dlb

2018-05-18 Thread Kieran Bingham
From: Kieran Bingham 

We are now able to configure a pipeline directly into a local display
list body. Take advantage of this fact, and create a cacheable body to
store the configuration of the pipeline in the video object.

vsp1_video_pipeline_run() is now the last user of the pipe->dl object.
Convert this function to use the cached video->config body and obtain a
local display list reference.

Attach the video->stream_config body to the display list when needed
before committing to hardware.

Use a flag 'configured' to know when we should attach our stream_config
to the next outgoing display list to reconfigure the hardware in the
event of our first frame, or the first frame following a suspend/resume
cycle.

Our video DL usage now looks like the below output:

dl->body0 contains our disposable runtime configuration. Max 41.
dl_child->body0 is our partition specific configuration. Max 12.
dl->bodies shows our constant configuration and LUTs.

  These two are LUT/CLU:
 * dl->bodies[x]->num_entries 256 / max 256
 * dl->bodies[x]->num_entries 4914 / max 4914

Which shows that our 'constant' configuration cache is currently
utilised to a maximum of 64 entries.

trace-cmd report | \
grep max | sed 's/.*vsp1_dl_list_commit://g' | sort | uniq;

  dl->body0->num_entries 13 / max 128
  dl->body0->num_entries 14 / max 128
  dl->body0->num_entries 16 / max 128
  dl->body0->num_entries 20 / max 128
  dl->body0->num_entries 27 / max 128
  dl->body0->num_entries 34 / max 128
  dl->body0->num_entries 41 / max 128
  dl_child->body0->num_entries 10 / max 128
  dl_child->body0->num_entries 12 / max 128
  dl->bodies[x]->num_entries 15 / max 128
  dl->bodies[x]->num_entries 16 / max 128
  dl->bodies[x]->num_entries 17 / max 128
  dl->bodies[x]->num_entries 18 / max 128
  dl->bodies[x]->num_entries 20 / max 128
  dl->bodies[x]->num_entries 21 / max 128
  dl->bodies[x]->num_entries 256 / max 256
  dl->bodies[x]->num_entries 31 / max 128
  dl->bodies[x]->num_entries 32 / max 128
  dl->bodies[x]->num_entries 39 / max 128
  dl->bodies[x]->num_entries 40 / max 128
  dl->bodies[x]->num_entries 47 / max 128
  dl->bodies[x]->num_entries 48 / max 128
  dl->bodies[x]->num_entries 4914 / max 4914
  dl->bodies[x]->num_entries 55 / max 128
  dl->bodies[x]->num_entries 56 / max 128
  dl->bodies[x]->num_entries 63 / max 128
  dl->bodies[x]->num_entries 64 / max 128

Signed-off-by: Kieran Bingham 
---
v11:
 - Remove dlbs pool from video object.
   Utilise the DLM pool for video->stream_config
 - Improve comments
 - clear the video->stream_config after it is released
   object.
 - stream_config and configured flag return to the pipe object.

v10:
 - Removed pipe->configured flag, and use
   pipe->state == VSP1_PIPELINE_STOPPED instead.

v8:
 - Fix comments
 - Rename video->pipe_config -> video->stream_config

v4:
 - Adjust pipe configured flag to be reset on resume rather than suspend
 - rename dl_child, dl_next

v3:
 - 's/fragment/body/', 's/fragments/bodies/'
 - video dlb cache allocation increased from 2 to 3 dlbs

 drivers/media/platform/vsp1/vsp1_dl.c| 10 +++-
 drivers/media/platform/vsp1/vsp1_dl.h|  1 +-
 drivers/media/platform/vsp1/vsp1_pipe.h  |  6 +-
 drivers/media/platform/vsp1/vsp1_video.c | 69 +++--
 4 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index c7fa1cb088cd..0f97305de965 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -813,6 +813,11 @@ void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
dlm->pending = NULL;
 }
 
+struct vsp1_dl_body *vsp1_dlm_dl_body_get(struct vsp1_dl_manager *dlm)
+{
+   return vsp1_dl_body_get(dlm->pool);
+}
+
 struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
unsigned int index,
unsigned int prealloc)
@@ -838,13 +843,14 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct 
vsp1_device *vsp1,
 * Initialize the display list body and allocate DMA memory for the body
 * and the optional header. Both are allocated together to avoid memory
 * fragmentation, with the header located right after the body in
-* memory.
+* memory. An extra body is allocated on top of the prealloc to account
+* for the cached body used by the vsp1_video object.
 */
header_size = dlm->mode == VSP1_DL_MODE_HEADER
? ALIGN(sizeof(struct vsp1_dl_header), 8)
: 0;
 
-   dlm->pool = vsp1_dl_body_pool_create(vsp1, prealloc,
+   dlm->pool = vsp1_dl_body_pool_create(vsp1, prealloc + 1,
 VSP1_DL_NUM_ENTRIES, header_size);
if (!dlm->pool)
return NULL;
diff --git