Re: [PATCH v9 6/8] media: vsp1: Refactor display list configure operations

2018-05-17 Thread Kieran Bingham 
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 
>>
>> ---
>> 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(>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(>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(>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(>flip.lock, flags);
>> +wpf->flip.active = (wpf->flip.active & ~mask)
>> + | (wpf->flip.pending & mask);
>> +spin_unlock_irqrestore(>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

Re: [PATCH v9 6/8] media: vsp1: Refactor display list configure operations

2018-05-17 Thread Laurent Pinchart 
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 
> 
> ---
> 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(>subdev);
> + struct v4l2_mbus_framefmt *format;
> 

I would have kept this blank line before the function.

> -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(>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(>subdev);
> + unsigned long flags;
> + u32 outfmt = 0;

No need to initialize outfmt to 0.

> +

This blank line isn't needed.

> + const unsigned int mask = BIT(WPF_CTRL_VFLIP)
> + | BIT(WPF_CTRL_HFLIP);
> +
> + spin_lock_irqsave(>flip.lock, flags);
> + wpf->flip.active = (wpf->flip.active & ~mask)
> +  | (wpf->flip.pending & mask);
> + spin_unlock_irqrestore(>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 

If you agree with those small changes there's no need to resubmit, I'll fix 
when applying.

-- 
Regards,

Laurent Pinchart

[PATCH v9 6/8] media: vsp1: Refactor display list configure operations

2018-05-03 Thread Kieran Bingham 
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 

---
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(-)

diff --git a/drivers/media/platform/vsp1/vsp1_brx.c 
b/drivers/media/platform/vsp1/vsp1_brx.c
index 3beec18fd863..011edac5ebc1 100644
--- a/drivers/media/platform/vsp1/vsp1_brx.c
+++ b/drivers/media/platform/vsp1/vsp1_brx.c
@@ -281,19 +281,15 @@ static const struct v4l2_subdev_ops brx_ops = {
  * VSP1 Entity Operations
  */
 
-static void brx_configure(struct vsp1_entity *entity,
- struct vsp1_pipeline *pipe,
- struct vsp1_dl_list *dl,
- enum vsp1_entity_params params)
+static void brx_configure_stream(struct vsp1_entity *entity,
+struct vsp1_pipeline *pipe,
+struct vsp1_dl_list *dl)
 {
struct vsp1_brx *brx = to_brx(>subdev);
struct v4l2_mbus_framefmt *format;
unsigned int flags;
unsigned int i;
 
-   if (params != VSP1_ENTITY_PARAMS_INIT)
-   return;
-
format = vsp1_entity_get_pad_format(>entity, brx->entity.config,
brx->entity.source_pad);
 
@@ -400,7 +396,7 @@ static void brx_configure(struct vsp1_entity *entity,
 }
 
 static const struct vsp1_entity_operations brx_entity_ops = {
-   .configure = brx_configure,
+   .configure_stream = brx_configure_stream,
 };
 
 /* 
-
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(>subdev);
+   struct v4l2_mbus_framefmt *format;
 
-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(>entity,
+   clu->entity.config,
+   CLU_PAD_SINK);
+   clu->yuv_mode = format->code == MEDIA_BUS_FMT_AYUV8_1X32;
+}
+
+static void clu_configure_frame(struct vsp1_entity *entity,
+   struct vsp1_pipeline *pipe,
+   struct vsp1_dl_list *dl)
 {
struct vsp1_clu *clu = to_clu(>subdev);
struct vsp1_dl_body *dlb;
unsigned long flags;
u32 ctrl = VI6_CLU_CTRL_AAI | VI6_CLU_CTRL_MVS | VI6_CLU_CTRL_EN;
 
-   switch (params) {
-   case