Hi Laurent,
On 13/02/17 19:17, Laurent Pinchart wrote:
> Hi Kieran,
>
> (CC'ing Morimoto-san)
>
> Thank you for the patch.
>
> On Friday 04 Nov 2016 18:19:27 Kieran Bingham wrote:
>> The partition algorithm introduced to support scaling, and rotation on
>
> s/,// if I'm not mistaken.
>
>> Gen3 hardware has some restrictions on pipeline configuration.
>
> Strictly speaking this shouldn't be a limitation of the partition algorithm,
> but a limitation of the Gen3 hardware. The limitations relate to image
> partitioning as partitions are needed when using the UDS or SRU in a pipeline,
> but as far as I understand, that's the whole extent of the relationship
> between the two.
How about swapping those two sentences over:
Gen3 hardware has some restrictions on pipeline configuration when using the
partition algorithm.
>> The UDS must not be connected after the SRU in a pipeline, and whilst an
>> SRU can be connected before the UDS, it can only do so in identity mode.
>
> I assume you mean after instead of before.
Indeed - I'm rather contradicting myself in the above sentence otherwise :)
> I've tested the SRU-UDS and UDS-SRU pipelines with all combinations for
> downscaling, identity and upscaling for the UDS and identity and upscaling for
> the UDS. On Gen2 everything works correctly.
>
> On Gen3 the results are as follows:
>
> Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in RGB24: fail
> Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in YUV444M: fail
> Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in YUV444M: hangs
> Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in YUV444M: fail
> Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in YUV444M: hangs
<longshot>
Do you still have the patch to vsp-tests for all these extra tests/checks?
</longshot>
> Two of the tests hang the VSP completely, for a reason I don't understand yet.
> Among the three tests that fail, the "UDS-SRU scaling 768x576 - 640x480 -
> 1280x960 in YUV444M" tests is a false positives due to differences in scaling
> algorithms between the VSP and gen-image.
Did the YUV444M tests hang repeatedly on the same test? I have seen 'random'
hangs on seemingly correct pipelines with YUV444M ... But reset/repeat test -
and it will work again. Most distressing, but perhaps might need deeper
investigation with the hardware.
> The only two tests that really fail are thus "SRU-UDS scaling 768x576 -
> 768x576 - 640x480" in RGB24 and YUV444M.
>
> The "20151116_VSP-IMG_Image_Partition_Information.pptx" document mentions the
> following restrictions:
>
> - double-scaled super resolution and UDS cannot be used at same time.
> - UDS cannot be connected after SRU.
>
> However, from the above tests it looks like the hardware can live with more
> relaxed restrictions than the ones implemented here. I haven't tested all UDS
> scaling ratios, and certainly not under all memory bus load conditions, I
> might thus be too optimistic. Morimoto-san, would it be possible to get more
> information about this from the hardware team, to check whether the above two
> restrictions need to be honoured, or whether they come from an older hardware
> version ?
Well, the updated documentation is certainly more descriptive than the first set
I saw when I started this. So I'll try to look at how I can update in this regards.
My theory would have been that the SRU / UDS can be used in combination, as long
as the input/output requirements are correctly met for both cells, whilst taking
into consideration the scaling up or down that they perform.
Thus I had thought that perhaps this restriction could be relaxed in the code
once we have 'negotiation' of the partition between the entities ... however
your testing shows SRUx1 -> UDS as the fail case, which I would have expected to
be an 'easy pass'
Perhaps later we might investigate if it can be worked around in software, but
as the documentation states it is unsupported, and we have (even if it's just
one) failure case matching that statement - perhaps it's best to provide the
limitation for now.
Morimoto, was there any further feedback from the hardware engineers on this
topic? (other than the updated documentation/datasheet)
>> On Gen3 hardware, the use of an SRU will always engage the partition
>> algorithm, therefore we must always ensure the restrictions are met on
>> Gen3 hardware utilising an SRU in the pipeline.
>>
>> A pipeline with an SRU connected after the UDS will disable any scaling
>> features of the SRU.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/media/platform/vsp1/vsp1_sru.c | 7 +++++--
>> drivers/media/platform/vsp1/vsp1_sru.h | 1 +
>> drivers/media/platform/vsp1/vsp1_video.c | 29 ++++++++++++++++++++++++++++-
>> 3 files changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_sru.c
>> b/drivers/media/platform/vsp1/vsp1_sru.c index b4e568a3b4ed..42a3ed6d9461
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_sru.c
>> +++ b/drivers/media/platform/vsp1/vsp1_sru.c
>> @@ -152,7 +152,8 @@ static int sru_enum_frame_size(struct v4l2_subdev
>> *subdev, fse->min_width = format->width;
>> fse->min_height = format->height;
>> if (format->width <= SRU_MAX_SIZE / 2 &&
>> - format->height <= SRU_MAX_SIZE / 2) {
>> + format->height <= SRU_MAX_SIZE / 2 &&
>> + sru->force_identity_mode == false) {
>
> This won't work, as force_identity_mode is set when starting the pipeline,
> while sru_enum_frame_size() and sru_try_format() are called before, when
> configuring the pipeline. I see no other option than failing pipeline
> validation instead of trying to report the problem to the user when setting
> formats (but feel free to prove me wrong).
Good spot.
I think you're right. Of course the links are already defined at this point, so
perhaps we could parse the media-device graph - but that seems a bit ugly/overkill.
>
>> fse->max_width = format->width * 2;
>> fse->max_height = format->height * 2;
>> } else {
>> @@ -203,7 +204,8 @@ static void sru_try_format(struct vsp1_sru *sru,
>>
>> if (fmt->width <= SRU_MAX_SIZE / 2 &&
>> fmt->height <= SRU_MAX_SIZE / 2 &&
>> - output_area > input_area * 9 / 4) {
>> + output_area > input_area * 9 / 4 &&
>> + sru->force_identity_mode == false) {
>> fmt->width = format->width * 2;
>> fmt->height = format->height * 2;
>> } else {
>> @@ -355,6 +357,7 @@ struct vsp1_sru *vsp1_sru_create(struct vsp1_device
>> *vsp1) v4l2_ctrl_new_custom(&sru->ctrls, &sru_intensity_control, NULL);
>>
>> sru->intensity = 1;
>> + sru->force_identity_mode = false;
>>
>> sru->entity.subdev.ctrl_handler = &sru->ctrls;
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_sru.h
>> b/drivers/media/platform/vsp1/vsp1_sru.h index 85e241457af2..f8652c04268e
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_sru.h
>> +++ b/drivers/media/platform/vsp1/vsp1_sru.h
>> @@ -30,6 +30,7 @@ struct vsp1_sru {
>> struct v4l2_ctrl_handler ctrls;
>>
>> unsigned int intensity;
>> + bool force_identity_mode;
>> };
>>
>> static inline struct vsp1_sru *to_sru(struct v4l2_subdev *subdev)
>> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
>> b/drivers/media/platform/vsp1/vsp1_video.c index f19d879ce5ee..d1d3413c6fdf
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_video.c
>> +++ b/drivers/media/platform/vsp1/vsp1_video.c
>> @@ -35,6 +35,7 @@
>> #include "vsp1_hgt.h"
>> #include "vsp1_pipe.h"
>> #include "vsp1_rwpf.h"
>> +#include "vsp1_sru.h"
>> #include "vsp1_uds.h"
>> #include "vsp1_video.h"
>>
>> @@ -458,10 +459,12 @@ static int vsp1_video_pipeline_build_branch(struct
>> vsp1_pipeline *pipe, struct vsp1_rwpf *input,
>> struct vsp1_rwpf *output)
>> {
>> + struct vsp1_device *vsp1 = output->entity.vsp1;
>> struct media_entity_enum ent_enum;
>> struct vsp1_entity *entity;
>> struct media_pad *pad;
>> bool bru_found = false;
>> + bool sru_found = false;
>> int ret;
>>
>> ret = media_entity_enum_init(&ent_enum, &input->entity.vsp1-
>> media_dev);
>> @@ -512,13 +515,37 @@ static int vsp1_video_pipeline_build_branch(struct
>> vsp1_pipeline *pipe, goto out;
>> }
>>
>> - /* UDS can't be chained. */
>> + if (entity->type == VSP1_ENTITY_SRU) {
>> + struct vsp1_sru *sru = to_sru(&entity->subdev);
>> +
>> + /*
>> + * Gen3 partition algorithm restricts SRU double-
> scaled
>> + * resolution if it is connected after a UDS entity
>> + */
>> + if (vsp1->info->gen == 3 && pipe->uds)
>> + sru->force_identity_mode = true;
>
> force_identity_mode is never reset to false.
it is in vsp1_sru_create(),
Aha, but vsp1_sru_create() is when the entities are created - and the pipeline
could be reconfigured/relinked.
I guess I should reset it at the beginning of the pipeline_build operation.
>
>> +
>> + sru_found = true;
>> + }
>> +
>> if (entity->type == VSP1_ENTITY_UDS) {
>> + /* UDS can't be chained. */
>> if (pipe->uds) {
>> ret = -EPIPE;
>> goto out;
>> }
>>
>> + /*
>> + * On Gen3 hardware using the partition algorithm, the
>> + * UDS must not be connected after the SRU. Using the
>> + * SRU on Gen3 will always engage the partition
>> + * algorithm
>> + */
>> + if (vsp1->info->gen == 3 && sru_found) {
>> + ret = -EPIPE;
>> + goto out;
>> + }
>> +
>> pipe->uds = entity;
>> pipe->uds_input = bru_found ? pipe->bru
>> : &input->entity;
--
Kieran
