Re: [PATCH v2 7/8] v4l: vsp1: Move video configuration to a cached dlb

2017-11-16 Thread Kieran Bingham 
Hi Laurent,

Thankyou for the review, and your patience on the long awaited response on these
remaining patches.

On 17/08/17 19:10, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Monday 14 Aug 2017 16:13:30 Kieran Bingham wrote:
>> 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->config body to the display list when needed before
>> committing to hardware.
>>
>> The pipe object is marked as un-configured when entering a suspend. This
>> ensures that upon resume, where the hardware is reset - our cached
>> configuration will be re-attached to the next committed DL.
>>
>> Signed-off-by: Kieran Bingham 
>> ---
>>
>> 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->fragments shows our constant configuration and LUTs.
>>
>>   These two are LUT/CLU:
>>  * dl->fragments[x]->num_entries 256 / max 256
>>  * dl->fragments[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->fragments[x]->num_entries 15 / max 128
>>   dl->fragments[x]->num_entries 16 / max 128
>>   dl->fragments[x]->num_entries 17 / max 128
>>   dl->fragments[x]->num_entries 18 / max 128
>>   dl->fragments[x]->num_entries 20 / max 128
>>   dl->fragments[x]->num_entries 21 / max 128
>>   dl->fragments[x]->num_entries 256 / max 256
>>   dl->fragments[x]->num_entries 31 / max 128
>>   dl->fragments[x]->num_entries 32 / max 128
>>   dl->fragments[x]->num_entries 39 / max 128
>>   dl->fragments[x]->num_entries 40 / max 128
>>   dl->fragments[x]->num_entries 47 / max 128
>>   dl->fragments[x]->num_entries 48 / max 128
>>   dl->fragments[x]->num_entries 4914 / max 4914
>>   dl->fragments[x]->num_entries 55 / max 128
>>   dl->fragments[x]->num_entries 56 / max 128
>>   dl->fragments[x]->num_entries 63 / max 128
>>   dl->fragments[x]->num_entries 64 / max 128
>> ---
>>  drivers/media/platform/vsp1/vsp1_pipe.c  |  4 +-
>>  drivers/media/platform/vsp1/vsp1_pipe.h  |  4 +-
>>  drivers/media/platform/vsp1/vsp1_video.c | 67 -
>>  drivers/media/platform/vsp1/vsp1_video.h |  2 +-
>>  4 files changed, 51 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
>> b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..7d1f7ba43060
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
>> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
>> @@ -249,6 +249,7 @@ void vsp1_pipeline_run(struct vsp1_pipeline *pipe)
>>  vsp1_write(vsp1, VI6_CMD(pipe->output->entity.index),
>> VI6_CMD_STRCMD);
>>  pipe->state = VSP1_PIPELINE_RUNNING;
>> +pipe->configured = true;
>>  }
>>
>>  pipe->buffers_ready = 0;
>> @@ -430,6 +431,9 @@ void vsp1_pipelines_suspend(struct vsp1_device *vsp1)
>>  spin_lock_irqsave(>irqlock, flags);
>>  if (pipe->state == VSP1_PIPELINE_RUNNING)
>>  pipe->state = VSP1_PIPELINE_STOPPING;
>> +
>> +/* After a suspend, the hardware will be reset */
>> +pipe->configured = false;
> 
> It shouldn't make a difference in practice, but I think it would be more 
> logical to set the configured field to false after the hardware has been 
> reset. I'd move this to the resume handler and update the comment to "The 
> hardware might have been reset during suspend and need a full 
> reconfiguration". 

Agreed, and Done.


> 
>>  spin_unlock_irqrestore(>irqlock, flags);
>>  }
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
>> b/drivers/media/platform/vsp1/vsp1_pipe.h index 90d29492b9b9..e7ad6211b4d0
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
>> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
>> @@ -90,6 +90,7 @@ struct vsp1_partition {
>>   * @irqlock: protects the pipeline state
>>   * @state: current state
>>   * @wq: wait queue to wait for state change completion
>> + * @configured: flag

Re: [PATCH v2 7/8] v4l: vsp1: Move video configuration to a cached dlb

2017-08-17 Thread Laurent Pinchart 
Hi Kieran,

Thank you for the patch.

On Monday 14 Aug 2017 16:13:30 Kieran Bingham wrote:
> 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->config body to the display list when needed before
> committing to hardware.
> 
> The pipe object is marked as un-configured when entering a suspend. This
> ensures that upon resume, where the hardware is reset - our cached
> configuration will be re-attached to the next committed DL.
> 
> Signed-off-by: Kieran Bingham 
> ---
> 
> 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->fragments shows our constant configuration and LUTs.
> 
>   These two are LUT/CLU:
>  * dl->fragments[x]->num_entries 256 / max 256
>  * dl->fragments[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->fragments[x]->num_entries 15 / max 128
>   dl->fragments[x]->num_entries 16 / max 128
>   dl->fragments[x]->num_entries 17 / max 128
>   dl->fragments[x]->num_entries 18 / max 128
>   dl->fragments[x]->num_entries 20 / max 128
>   dl->fragments[x]->num_entries 21 / max 128
>   dl->fragments[x]->num_entries 256 / max 256
>   dl->fragments[x]->num_entries 31 / max 128
>   dl->fragments[x]->num_entries 32 / max 128
>   dl->fragments[x]->num_entries 39 / max 128
>   dl->fragments[x]->num_entries 40 / max 128
>   dl->fragments[x]->num_entries 47 / max 128
>   dl->fragments[x]->num_entries 48 / max 128
>   dl->fragments[x]->num_entries 4914 / max 4914
>   dl->fragments[x]->num_entries 55 / max 128
>   dl->fragments[x]->num_entries 56 / max 128
>   dl->fragments[x]->num_entries 63 / max 128
>   dl->fragments[x]->num_entries 64 / max 128
> ---
>  drivers/media/platform/vsp1/vsp1_pipe.c  |  4 +-
>  drivers/media/platform/vsp1/vsp1_pipe.h  |  4 +-
>  drivers/media/platform/vsp1/vsp1_video.c | 67 -
>  drivers/media/platform/vsp1/vsp1_video.h |  2 +-
>  4 files changed, 51 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..7d1f7ba43060
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -249,6 +249,7 @@ void vsp1_pipeline_run(struct vsp1_pipeline *pipe)
>   vsp1_write(vsp1, VI6_CMD(pipe->output->entity.index),
>  VI6_CMD_STRCMD);
>   pipe->state = VSP1_PIPELINE_RUNNING;
> + pipe->configured = true;
>   }
> 
>   pipe->buffers_ready = 0;
> @@ -430,6 +431,9 @@ void vsp1_pipelines_suspend(struct vsp1_device *vsp1)
>   spin_lock_irqsave(>irqlock, flags);
>   if (pipe->state == VSP1_PIPELINE_RUNNING)
>   pipe->state = VSP1_PIPELINE_STOPPING;
> +
> + /* After a suspend, the hardware will be reset */
> + pipe->configured = false;

It shouldn't make a difference in practice, but I think it would be more 
logical to set the configured field to false after the hardware has been 
reset. I'd move this to the resume handler and update the comment to "The 
hardware might have been reset during suspend and need a full 
reconfiguration". 

>   spin_unlock_irqrestore(>irqlock, flags);
>   }
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index 90d29492b9b9..e7ad6211b4d0
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -90,6 +90,7 @@ struct vsp1_partition {
>   * @irqlock: protects the pipeline state
>   * @state: current state
>   * @wq: wait queue to wait for state change completion
> + * @configured: flag determining if the hardware has run since reset
>   * @frame_end: frame end interrupt handler
>   * @lock: protects the pipeline use count and stream count
>   * @kref: pipeline reference count
> @@ -117,6 +118,7 @@ struct vsp1_pipeline {
>   spinlock_t irqlock;
>

[PATCH v2 7/8] v4l: vsp1: Move video configuration to a cached dlb

2017-08-14 Thread 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->config body to the display list when needed before
committing to hardware.

The pipe object is marked as un-configured when entering a suspend. This
ensures that upon resume, where the hardware is reset - our cached
configuration will be re-attached to the next committed DL.

Signed-off-by: Kieran Bingham 
---

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->fragments shows our constant configuration and LUTs.

  These two are LUT/CLU:
 * dl->fragments[x]->num_entries 256 / max 256
 * dl->fragments[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->fragments[x]->num_entries 15 / max 128
  dl->fragments[x]->num_entries 16 / max 128
  dl->fragments[x]->num_entries 17 / max 128
  dl->fragments[x]->num_entries 18 / max 128
  dl->fragments[x]->num_entries 20 / max 128
  dl->fragments[x]->num_entries 21 / max 128
  dl->fragments[x]->num_entries 256 / max 256
  dl->fragments[x]->num_entries 31 / max 128
  dl->fragments[x]->num_entries 32 / max 128
  dl->fragments[x]->num_entries 39 / max 128
  dl->fragments[x]->num_entries 40 / max 128
  dl->fragments[x]->num_entries 47 / max 128
  dl->fragments[x]->num_entries 48 / max 128
  dl->fragments[x]->num_entries 4914 / max 4914
  dl->fragments[x]->num_entries 55 / max 128
  dl->fragments[x]->num_entries 56 / max 128
  dl->fragments[x]->num_entries 63 / max 128
  dl->fragments[x]->num_entries 64 / max 128
---
 drivers/media/platform/vsp1/vsp1_pipe.c  |  4 +-
 drivers/media/platform/vsp1/vsp1_pipe.h  |  4 +-
 drivers/media/platform/vsp1/vsp1_video.c | 67 -
 drivers/media/platform/vsp1/vsp1_video.h |  2 +-
 4 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c 
b/drivers/media/platform/vsp1/vsp1_pipe.c
index 5012643583b6..7d1f7ba43060 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -249,6 +249,7 @@ void vsp1_pipeline_run(struct vsp1_pipeline *pipe)
vsp1_write(vsp1, VI6_CMD(pipe->output->entity.index),
   VI6_CMD_STRCMD);
pipe->state = VSP1_PIPELINE_RUNNING;
+   pipe->configured = true;
}
 
pipe->buffers_ready = 0;
@@ -430,6 +431,9 @@ void vsp1_pipelines_suspend(struct vsp1_device *vsp1)
spin_lock_irqsave(>irqlock, flags);
if (pipe->state == VSP1_PIPELINE_RUNNING)
pipe->state = VSP1_PIPELINE_STOPPING;
+
+   /* After a suspend, the hardware will be reset */
+   pipe->configured = false;
spin_unlock_irqrestore(>irqlock, flags);
}
 
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h 
b/drivers/media/platform/vsp1/vsp1_pipe.h
index 90d29492b9b9..e7ad6211b4d0 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -90,6 +90,7 @@ struct vsp1_partition {
  * @irqlock: protects the pipeline state
  * @state: current state
  * @wq: wait queue to wait for state change completion
+ * @configured: flag determining if the hardware has run since reset
  * @frame_end: frame end interrupt handler
  * @lock: protects the pipeline use count and stream count
  * @kref: pipeline reference count
@@ -117,6 +118,7 @@ struct vsp1_pipeline {
spinlock_t irqlock;
enum vsp1_pipeline_state state;
wait_queue_head_t wq;
+   bool configured;
 
void (*frame_end)(struct vsp1_pipeline *pipe, bool completed);
 
@@ -143,8 +145,6 @@ struct vsp1_pipeline {
 */
struct list_head entities;
 
-   struct vsp1_dl_list *dl;
-
unsigned int partitions;
struct vsp1_partition *partition;
struct vsp1_partition *part_table;
diff --git a/drivers/media/platform/vsp1/vsp1_video.c 
b/drivers/media/platform/vsp1/vsp1_video.c
index 7e825f3360bf..42b70b8465ba 100644
---