Re: [PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb
Hi Kieran, On Thursday, 17 May 2018 20:06:46 EEST Kieran Bingham wrote: > On 17/05/18 15:35, Laurent Pinchart wrote: > > On Monday, 30 April 2018 20:48:03 EEST Kieran Bingham wrote: > >> On 07/04/18 01:23, Laurent Pinchart wrote: > >>> On Thursday, 8 March 2018 02:05:31 EEST 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 resuming from a > suspend. This ensures that when the hardware is reset - our cached > configuration will be re-attached to the next committed DL. > > Signed-off-by: Kieran Bingham > --- > > v3: > - 's/fragment/body/', 's/fragments/bodies/' > - video dlb cache allocation increased from 2 to 3 dlbs > > 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 > >>> > >>> This might be useful to capture in the main part of the commit message. > >>> > v4: > - Adjust pipe configured flag to be reset on resume rather than > suspend > - rename dl_child, dl_next > > drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- > 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, 54 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c > b/drivers/media/platform/vsp1/vsp1_pipe.c index > 5012643583b6..fa445b1a2e38 > 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; > @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device > *vsp1) > continue; > > spin_lock_irqsave(&pipe->irqlock, flags); > +/* > + * The hardware may have been reset during a suspend > and will > + * need a full reconfiguration > + */ > >>> > >>> s/reconfiguration/reconfiguration./ > >>> > +pipe->configured = false; > + > >>> >
Re: [PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb
Hi Laurent, On 17/05/18 15:35, Laurent Pinchart wrote: > Hi Kieran, > > On Monday, 30 April 2018 20:48:03 EEST Kieran Bingham wrote: >> On 07/04/18 01:23, Laurent Pinchart wrote: >>> On Thursday, 8 March 2018 02:05:31 EEST 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 resuming from a suspend. This ensures that when the hardware is reset - our cached configuration will be re-attached to the next committed DL. Signed-off-by: Kieran Bingham --- v3: - 's/fragment/body/', 's/fragments/bodies/' - video dlb cache allocation increased from 2 to 3 dlbs 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 >>> >>> This might be useful to capture in the main part of the commit message. >>> v4: - Adjust pipe configured flag to be reset on resume rather than suspend - rename dl_child, dl_next drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- 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, 54 insertions(+), 26 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..fa445b1a2e38 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; @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1) continue; spin_lock_irqsave(&pipe->irqlock, flags); + /* + * The hardware may have been reset during a suspend and will + * need a full reconfiguration + */ >>> >>> s/reconfiguration/reconfiguration./ >>> + pipe->configured = false; + >>> >>> Where does that full reconfiguration occur, given that the >>> vsp1_pipeline_run() right below sets pipe->configured to true without >>> performing reconfiguration ? > Q >> It's magic isn't it :D >> >> If the pipe->configured flag gets set to false, the next execution of >> vsp1_pipeline_run() attaches the video->pipe_config (the cached >> configuration, containing the route_setup() and
Re: [PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb
Hi Kieran, On Monday, 30 April 2018 20:48:03 EEST Kieran Bingham wrote: > On 07/04/18 01:23, Laurent Pinchart wrote: > > On Thursday, 8 March 2018 02:05:31 EEST 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 resuming from a suspend. > >> This ensures that when the hardware is reset - our cached configuration > >> will be re-attached to the next committed DL. > >> > >> Signed-off-by: Kieran Bingham > >> --- > >> > >> v3: > >> - 's/fragment/body/', 's/fragments/bodies/' > >> - video dlb cache allocation increased from 2 to 3 dlbs > >> > >> 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 > > > > This might be useful to capture in the main part of the commit message. > > > >> v4: > >> - Adjust pipe configured flag to be reset on resume rather than suspend > >> - rename dl_child, dl_next > >> > >> drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- > >> 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, 54 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c > >> b/drivers/media/platform/vsp1/vsp1_pipe.c index > >> 5012643583b6..fa445b1a2e38 > >> 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; > >> @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1) > >>continue; > >> > >>spin_lock_irqsave(&pipe->irqlock, flags); > >> + /* > >> + * The hardware may have been reset during a suspend and will > >> + * need a full reconfiguration > >> + */ > > > > s/reconfiguration/reconfiguration./ > > > >> + pipe->configured = false; > >> + > > > > Where does that full reconfiguration occur, given that the > > vsp1_pipeline_run() right below sets pipe->configured to true without > > performing reconfiguration ? Q > It's magic isn't it :D > > If the pipe->configured flag gets set to false, the next execution of > vsp1_pipeline_run() attaches the video->pipe_config (the cached > configuration, containing the route_setup() and the configure_stream() > entries) to the display lis
Re: [PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 On 01/05/18 09:28, Kieran Bingham wrote: > Hi Laurent, > > New plan ... (from the .. why didn't I think of this earlier department) Nope ... My suggestion in the previous mail (regarding dropping the configured flag in place of using the pipe->state s) doesn't work because the pipeline is set to be running before the vsp1_video_pipeline_run() gets the opportunity to check it. We could extend the states to include a 'VSP1_PIPELINE_STARTING' in between _STOPPED, and _RUNNING, but I think that's more complicated as the state machine across the whole code will be affected. (which is why I chose to add a flag in the first place) > On 30/04/18 18:48, Kieran Bingham wrote: >> Hi Laurent, >> >> On 07/04/18 01:23, Laurent Pinchart wrote: >>> Hi Kieran, >>> >>> Thank you for the patch. >>> >>> On Thursday, 8 March 2018 02:05:31 EEST 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 resuming from a suspend. This ensures that when the hardware is reset - our cached configuration will be re-attached to the next committed DL. Signed-off-by: Kieran Bingham --- v3: - 's/fragment/body/', 's/fragments/bodies/' - video dlb cache allocation increased from 2 to 3 dlbs 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 >>> >>> This might be useful to capture in the main part of the commit >>> message. >>> v4: - Adjust pipe configured flag to be reset on resume rather than suspend - rename dl_child, dl_next drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- 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, 54 insertions(+), 26 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..fa445b1a2e38 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; > > Look at that lovely pipe->state flag update right above the > pipe->configured update... > } pipe->buffers_ready = 0; @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1) continue; spin_lock_irqsave(&pipe->irqlock, flags); +/* + * The hardware may have been reset during a suspend and will +
Re: [PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb
Hi Laurent, New plan ... (from the .. why didn't I think of this earlier department) On 30/04/18 18:48, Kieran Bingham wrote: > Hi Laurent, > > On 07/04/18 01:23, Laurent Pinchart wrote: >> Hi Kieran, >> >> Thank you for the patch. >> >> On Thursday, 8 March 2018 02:05:31 EEST 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 resuming from a suspend. >>> This ensures that when the hardware is reset - our cached configuration >>> will be re-attached to the next committed DL. >>> >>> Signed-off-by: Kieran Bingham >>> --- >>> >>> v3: >>> - 's/fragment/body/', 's/fragments/bodies/' >>> - video dlb cache allocation increased from 2 to 3 dlbs >>> >>> 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 >> >> This might be useful to capture in the main part of the commit message. >> >>> v4: >>> - Adjust pipe configured flag to be reset on resume rather than suspend >>> - rename dl_child, dl_next >>> >>> drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- >>> 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, 54 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c >>> b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..fa445b1a2e38 >>> 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; Look at that lovely pipe->state flag update right above the pipe->configured update... >>> } >>> >>> pipe->buffers_ready = 0; >>> @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1) >>> continue; >>> >>> spin_lock_irqsave(&pipe->irqlock, flags); >>> + /* >>> +* The hardware may have been reset during a suspend and will >>> +* need a full reconfiguration >>> +*/ >> >> s/reconfiguration/reconfiguration./ >> >>> + pipe->configured = false; If we have 'suspended' then pipe->state == STOPPED >>> + >> >> Where does that full reconfiguration occur, given that the >> vsp1_pipeline_run() >> right below sets pipe->configured to true without performing reconfiguration >> ? > > It's magic isn't it :D > > If the pipe->configured flag gets set to false, the next execution of > vsp1_pipeline_run() attaches the video-
Re: [PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb
Hi Laurent, On 07/04/18 01:23, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thursday, 8 March 2018 02:05:31 EEST 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 resuming from a suspend. >> This ensures that when the hardware is reset - our cached configuration >> will be re-attached to the next committed DL. >> >> Signed-off-by: Kieran Bingham >> --- >> >> v3: >> - 's/fragment/body/', 's/fragments/bodies/' >> - video dlb cache allocation increased from 2 to 3 dlbs >> >> 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 > > This might be useful to capture in the main part of the commit message. > >> v4: >> - Adjust pipe configured flag to be reset on resume rather than suspend >> - rename dl_child, dl_next >> >> drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- >> 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, 54 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c >> b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..fa445b1a2e38 >> 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; >> @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1) >> continue; >> >> spin_lock_irqsave(&pipe->irqlock, flags); >> +/* >> + * The hardware may have been reset during a suspend and will >> + * need a full reconfiguration >> + */ > > s/reconfiguration/reconfiguration./ > >> +pipe->configured = false; >> + > > Where does that full reconfiguration occur, given that the > vsp1_pipeline_run() > right below sets pipe->configured to true without performing reconfiguration ? It's magic isn't it :D If the pipe->configured flag gets set to false, the next execution of vsp1_pipeline_run() attaches the video->pipe_config (the cached configuration, containing the route_setup() and the configure_stream() entries) to the display list before configuring for the next frame. This means that the hardware gets a full configuration written to it after a suspend/resume action. Perhaps the comment should say "The video object will write out it's cached pipe configuration on the next display list
Re: [PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb
Hi Kieran, Thank you for the patch. On Thursday, 8 March 2018 02:05:31 EEST 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 resuming from a suspend. > This ensures that when the hardware is reset - our cached configuration > will be re-attached to the next committed DL. > > Signed-off-by: Kieran Bingham > --- > > v3: > - 's/fragment/body/', 's/fragments/bodies/' > - video dlb cache allocation increased from 2 to 3 dlbs > > 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 This might be useful to capture in the main part of the commit message. > v4: > - Adjust pipe configured flag to be reset on resume rather than suspend > - rename dl_child, dl_next > > drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- > 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, 54 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c > b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..fa445b1a2e38 > 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; > @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1) > continue; > > spin_lock_irqsave(&pipe->irqlock, flags); > + /* > + * The hardware may have been reset during a suspend and will > + * need a full reconfiguration > + */ s/reconfiguration/reconfiguration./ > + pipe->configured = false; > + Where does that full reconfiguration occur, given that the vsp1_pipeline_run() right below sets pipe->configured to true without performing reconfiguration ? > if (vsp1_pipeline_ready(pipe)) > vsp1_pipeline_run(pipe); > spin_unlock_irqrestore(&pipe->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 interr
[PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb
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 resuming from a suspend. This ensures that when the hardware is reset - our cached configuration will be re-attached to the next committed DL. Signed-off-by: Kieran Bingham --- v3: - 's/fragment/body/', 's/fragments/bodies/' - video dlb cache allocation increased from 2 to 3 dlbs 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 v4: - Adjust pipe configured flag to be reset on resume rather than suspend - rename dl_child, dl_next drivers/media/platform/vsp1/vsp1_pipe.c | 7 +++- 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, 54 insertions(+), 26 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..fa445b1a2e38 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; @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1) continue; spin_lock_irqsave(&pipe->irqlock, flags); + /* +* The hardware may have been reset during a suspend and will +* need a full reconfiguration +*/ + pipe->configured = false; + if (vsp1_pipeline_ready(pipe)) vsp1_pipeline_run(pipe); spin_unlock_irqrestore(&pipe->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; s