Hi Laurent,
Thanks for the review,
On 17/05/18 10:41, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Thursday, 3 May 2018 16:35:45 EEST Kieran Bingham wrote:
>> The entities provide a single .configure operation which configures the
>> object into the target display list, based on the vsp1_entity_params
>> selection.
>>
>> Split the configure function into three parts, '.configure_stream()',
>> '.configure_frame()', and '.configure_partition()' to facilitate
>> splitting the configuration of each parameter class into separate
>> display list bodies.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>>
>> ---
>> The checkpatch warning:
>>
>> WARNING: function definition argument 'struct vsp1_dl_list *' should
>> also have an identifier name
>>
>> has been ignored to match the existing code style.
>>
>> v8:
>> - Add support for the UIF
>> - Remove unrelated whitespace change
>> - Fix comment location for clu_configure_stream()
>> - Update configure documentations
>> - Implement configure_partition separation.
>>
>> v7
>> - Fix formatting and white space
>> - s/prepare/configure_stream/
>> - s/configure/configure_frame/
>> ---
>> drivers/media/platform/vsp1/vsp1_brx.c | 12 +-
>> drivers/media/platform/vsp1/vsp1_clu.c | 77 ++----
>> drivers/media/platform/vsp1/vsp1_drm.c | 12 +-
>> drivers/media/platform/vsp1/vsp1_entity.c | 24 ++-
>> drivers/media/platform/vsp1/vsp1_entity.h | 39 +--
>> drivers/media/platform/vsp1/vsp1_hgo.c | 12 +-
>> drivers/media/platform/vsp1/vsp1_hgt.c | 12 +-
>> drivers/media/platform/vsp1/vsp1_hsit.c | 12 +-
>> drivers/media/platform/vsp1/vsp1_lif.c | 12 +-
>> drivers/media/platform/vsp1/vsp1_lut.c | 47 +---
>> drivers/media/platform/vsp1/vsp1_rpf.c | 168 ++++++-------
>> drivers/media/platform/vsp1/vsp1_sru.c | 12 +-
>> drivers/media/platform/vsp1/vsp1_uds.c | 56 ++--
>> drivers/media/platform/vsp1/vsp1_uif.c | 16 +-
>> drivers/media/platform/vsp1/vsp1_video.c | 28 +--
>> drivers/media/platform/vsp1/vsp1_wpf.c | 303 ++++++++++++-----------
>> 16 files changed, 422 insertions(+), 420 deletions(-)
>
> [snip]
>
>> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
>> b/drivers/media/platform/vsp1/vsp1_clu.c index ea83f1b7d125..0a978980d447
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_clu.c
>> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
>> @@ -168,58 +168,50 @@ static const struct v4l2_subdev_ops clu_ops = {
>> /* ------------------------------------------------------------------------
>> * VSP1 Entity Operations
>> */
>> +static void clu_configure_stream(struct vsp1_entity *entity,
>> + struct vsp1_pipeline *pipe,
>> + struct vsp1_dl_list *dl)
>> +{
>> + struct vsp1_clu *clu = to_clu(&entity->subdev);
>> + struct v4l2_mbus_framefmt *format;
>>
>
> I would have kept this blank line before the function.
I would have thought I would too :)
>
>> -static void clu_configure(struct vsp1_entity *entity,
>> - struct vsp1_pipeline *pipe,
>> - struct vsp1_dl_list *dl,
>> - enum vsp1_entity_params params)
>> + /*
>> + * The yuv_mode can't be changed during streaming. Cache it internally
>> + * for future runtime configuration calls.
>> + */
>> + format = vsp1_entity_get_pad_format(&clu->entity,
>> + clu->entity.config,
>> + CLU_PAD_SINK);
>> + clu->yuv_mode = format->code == MEDIA_BUS_FMT_AYUV8_1X32;
>> +}
>
> [snip]
>
>> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
>> b/drivers/media/platform/vsp1/vsp1_wpf.c index 65ed2f849551..da287c27b324
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
>> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
>
> [snip]
>
>> +static void wpf_configure_frame(struct vsp1_entity *entity,
>> + struct vsp1_pipeline *pipe,
>> + struct vsp1_dl_list *dl)
>> +{
>> + struct vsp1_rwpf *wpf = to_rwpf(&entity->subdev);
>> + unsigned long flags;
>> + u32 outfmt = 0;
>
> No need to initialize outfmt to 0.
Ah yes, indeed!.
>> +
>
> This blank line isn't needed.
>
>> + const unsigned int mask = BIT(WPF_CTRL_VFLIP)
>> + | BIT(WPF_CTRL_HFLIP);
>> +
>> + spin_lock_irqsave(&wpf->flip.lock, flags);
>> + wpf->flip.active = (wpf->flip.active & ~mask)
>> + | (wpf->flip.pending & mask);
>> + spin_unlock_irqrestore(&wpf->flip.lock, flags);
>> +
>> + outfmt = (wpf->alpha << VI6_WPF_OUTFMT_PDV_SHIFT) | wpf->outfmt;
>> +
>> + if (wpf->flip.active & BIT(WPF_CTRL_VFLIP))
>> + outfmt |= VI6_WPF_OUTFMT_FLP;
>> + if (wpf->flip.active & BIT(WPF_CTRL_HFLIP))
>> + outfmt |= VI6_WPF_OUTFMT_HFLP;
>> +
>> + vsp1_wpf_write(wpf, dl, VI6_WPF_OUTFMT, outfmt);
>> +}
>
> [snip]
>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> If you agree with those small changes there's no need to resubmit, I'll fix
> when applying.
No objections to any white-space corrections you feel are necessary.
Thanks.
Kieran
