subject:"\[PATCH v2 6\/8\] v4l\: vsp1\: Add support for the DISCOM entity"

Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity

2018-04-28 Thread Kieran Bingham

Hi Laurent,

On 22/04/18 23:34, Laurent Pinchart wrote:
> The DISCOM calculates a CRC on a configurable window of the frame. It
> interfaces to the VSP through the UIF glue, hence the name used in the
> code.
> 
> The module supports configuration of the CRC window through the crop
> rectangle on the ink pad of the corresponding entity. However, unlike
> the traditional V4L2 subdevice model, the crop rectangle does not
> influence the format on the source pad.
> 
> Modeling the DISCOM as a sink-only entity would allow adhering to the
> V4L2 subdevice model at the expense of more complex code in the driver,
> as at the hardware level the UIF is handled as a sink+source entity. As
> the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it
> is not exposed to userspace through V4L2 but controlled through the DU
> driver. We can thus change this model later if needed without fear of
> affecting userspace.
> 
> Signed-off-by: Laurent Pinchart 

Registers check out against the data sheet, and I haven't spotted anything else
here on top of the comments Jacopo has highlighted.

I'll put a +1 on Jacopo's suggestion for handling the m3-w quirk though. It's an
adjustment to the width/height in the event the quirk is active - so adjusting
the values after they are set feels 'right' to me.


Reviewed-by: Kieran Bingham 



> ---
> Changes since v1:
> 
> - Don't return uninitialized value from uif_set_selection()
> ---
>  drivers/media/platform/vsp1/Makefile  |   2 +-
>  drivers/media/platform/vsp1/vsp1.h|   4 +
>  drivers/media/platform/vsp1/vsp1_drv.c|  20 +++
>  drivers/media/platform/vsp1/vsp1_entity.c |   6 +
>  drivers/media/platform/vsp1/vsp1_entity.h |   1 +
>  drivers/media/platform/vsp1/vsp1_regs.h   |  41 +
>  drivers/media/platform/vsp1/vsp1_uif.c| 271 
> ++
>  drivers/media/platform/vsp1/vsp1_uif.h|  32 
>  8 files changed, 376 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c
>  create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h
> 
> diff --git a/drivers/media/platform/vsp1/Makefile 
> b/drivers/media/platform/vsp1/Makefile
> index 596775f932c0..4bb4dcbef7b5 100644
> --- a/drivers/media/platform/vsp1/Makefile
> +++ b/drivers/media/platform/vsp1/Makefile
> @@ -5,6 +5,6 @@ vsp1-y+= vsp1_rpf.o 
> vsp1_rwpf.o vsp1_wpf.o
>  vsp1-y   += vsp1_clu.o vsp1_hsit.o 
> vsp1_lut.o
>  vsp1-y   += vsp1_brx.o vsp1_sru.o 
> vsp1_uds.o
>  vsp1-y   += vsp1_hgo.o vsp1_hgt.o 
> vsp1_histo.o
> -vsp1-y   += vsp1_lif.o
> +vsp1-y   += vsp1_lif.o vsp1_uif.o
>  
>  obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o
> diff --git a/drivers/media/platform/vsp1/vsp1.h 
> b/drivers/media/platform/vsp1/vsp1.h
> index 9cf4e1c4b036..33f632331474 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -36,10 +36,12 @@ struct vsp1_lut;
>  struct vsp1_rwpf;
>  struct vsp1_sru;
>  struct vsp1_uds;
> +struct vsp1_uif;
>  
>  #define VSP1_MAX_LIF 2
>  #define VSP1_MAX_RPF 5
>  #define VSP1_MAX_UDS 3
> +#define VSP1_MAX_UIF 2
>  #define VSP1_MAX_WPF 4
>  
>  #define VSP1_HAS_LUT (1 << 1)
> @@ -60,6 +62,7 @@ struct vsp1_device_info {
>   unsigned int lif_count;
>   unsigned int rpf_count;
>   unsigned int uds_count;
> + unsigned int uif_count;
>   unsigned int wpf_count;
>   unsigned int num_bru_inputs;
>   bool uapi;
> @@ -86,6 +89,7 @@ struct vsp1_device {
>   struct vsp1_rwpf *rpf[VSP1_MAX_RPF];
>   struct vsp1_sru *sru;
>   struct vsp1_uds *uds[VSP1_MAX_UDS];
> + struct vsp1_uif *uif[VSP1_MAX_UIF];
>   struct vsp1_rwpf *wpf[VSP1_MAX_WPF];
>  
>   struct list_head entities;
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 331a2e0af0d3..d29f9c4baebe 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -35,6 +35,7 @@
>  #include "vsp1_rwpf.h"
>  #include "vsp1_sru.h"
>  #include "vsp1_uds.h"
> +#include "vsp1_uif.h"
>  #include "vsp1_video.h"
>  
>  /* 
> -
> @@ -409,6 +410,19 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
>   list_add_tail(>entity.list_dev, >entities);
>   }
>  
> + for (i = 0; i < vsp1->info->uif_count; ++i) {
> + struct vsp1_uif *uif;
> +
> + uif = vsp1_uif_create(vsp1, i);
> + if (IS_ERR(uif)) {
> + ret = PTR_ERR(uif);
> + goto done;
> + }
> +
> + vsp1->uif[i]

Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity

2018-04-28 Thread Laurent Pinchart

Hi Jacopo,

On Saturday, 28 April 2018 13:40:02 EEST jacopo mondi wrote:
> HI Laurent,
>a few comments, mostly minor ones...
> 
> On Mon, Apr 23, 2018 at 01:34:28AM +0300, Laurent Pinchart wrote:
> > The DISCOM calculates a CRC on a configurable window of the frame. It
> > interfaces to the VSP through the UIF glue, hence the name used in the
> > code.
> > 
> > The module supports configuration of the CRC window through the crop
> > rectangle on the ink pad of the corresponding entity. However, unlike
> 
> sink pad?

Oops. Consider it fixed.

> > the traditional V4L2 subdevice model, the crop rectangle does not
> > influence the format on the source pad.
> > 
> > Modeling the DISCOM as a sink-only entity would allow adhering to the
> > V4L2 subdevice model at the expense of more complex code in the driver,
> > as at the hardware level the UIF is handled as a sink+source entity. As
> > the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it
> > is not exposed to userspace through V4L2 but controlled through the DU
> > driver. We can thus change this model later if needed without fear of
> > affecting userspace.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > ---
> > Changes since v1:
> > 
> > - Don't return uninitialized value from uif_set_selection()
> > ---
> > 
> >  drivers/media/platform/vsp1/Makefile  |   2 +-
> >  drivers/media/platform/vsp1/vsp1.h|   4 +
> >  drivers/media/platform/vsp1/vsp1_drv.c|  20 +++
> >  drivers/media/platform/vsp1/vsp1_entity.c |   6 +
> >  drivers/media/platform/vsp1/vsp1_entity.h |   1 +
> >  drivers/media/platform/vsp1/vsp1_regs.h   |  41 +
> >  drivers/media/platform/vsp1/vsp1_uif.c| 271 +
> >  drivers/media/platform/vsp1/vsp1_uif.h|  32 
> >  8 files changed, 376 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h

[snip]

> > diff --git a/drivers/media/platform/vsp1/vsp1_uif.c
> > b/drivers/media/platform/vsp1/vsp1_uif.c new file mode 100644
> > index ..6de7e9c801ae
> > --- /dev/null
> > +++ b/drivers/media/platform/vsp1/vsp1_uif.c
> > @@ -0,0 +1,271 @@

[snip]

> > +static void uif_configure(struct vsp1_entity *entity,
> > + struct vsp1_pipeline *pipe,
> > + struct vsp1_dl_list *dl,
> > + enum vsp1_entity_params params)
> > +{
> > +   struct vsp1_uif *uif = to_uif(>subdev);
> > +   const struct v4l2_rect *crop;
> > +   unsigned int left;
> > +   unsigned int width;
> > +
> > +   /*
> > +* Per-partition configuration isn't needed as the DISCOM is used in
> > +* display pipelines only.
> > +*/
> > +   if (params != VSP1_ENTITY_PARAMS_INIT)
> > +   return;
> > +
> > +   vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMPMR,
> > +  VI6_UIF_DISCOM_DOCMPMR_SEL(9));
> > +
> > +   crop = vsp1_entity_get_pad_selection(entity, entity->config,
> > +UIF_PAD_SINK, V4L2_SEL_TGT_CROP);
> > +
> > +   /* On M3-W the horizontal coordinates are twice the register value. */
> > +   if (uif->m3w_quirk) {
> > +   left = crop->left / 2;
> > +   width = crop->width / 2;
> > +   } else {
> > +   left = crop->left;
> > +   width = crop->width;
> > +   }
> 
> I would write this as
> 
> left = crop->left;
> width = crop->width;
>   /* On M3-W the horizontal coordinates are twice the register value. */
>   if (uif->m3w_quirk) {
>   left /= 2;
>   width /= 2;
> }
> 
> But that's really up to you.

I prefer my style, but it looks like gcc 6.4.0 generates slightly better code 
with your version (due to the fact that the crop->left value is converted to 
unsigned before being divided by 2), so I'll go for it.

> > +
> > +   vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSPXR, left);
> > +   vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSPYR, crop->top);
> > +   vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSZXR, width);
> > +   vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMSZYR, crop->height);
> > +
> > +   vsp1_uif_write(uif, dl, VI6_UIF_DISCOM_DOCMCR,
> > +  VI6_UIF_DISCOM_DOCMCR_CMPR);
> > +}
> > +
> > +static const struct vsp1_entity_operations uif_entity_ops = {
> > +   .configure = uif_configure,
> > +};
> > +
> > +/* --
> > + * Initialization and Cleanup
> > + */
> > +
> > +static const struct soc_device_attribute vsp1_r8a7796[] = {
> > +   { .soc_id = "r8a7796" },
> > +   { /* sentinel */ }
> > +};
> > +
> > +struct vsp1_uif *vsp1_uif_create(struct vsp1_device *vsp1, unsigned int
> > index) +{
> > +   struct vsp1_uif *uif;
> > +   char name[6];
> > +   int ret;
> > +
> > +   uif = devm_kzalloc(vsp1->dev, sizeof(*uif), GFP_KERNEL);
> > +   if (uif == NULL)
> 
> if (!uif)
>

Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity

2018-04-28 Thread jacopo mondi

HI Laurent,
   a few comments, mostly minor ones...

On Mon, Apr 23, 2018 at 01:34:28AM +0300, Laurent Pinchart wrote:
> The DISCOM calculates a CRC on a configurable window of the frame. It
> interfaces to the VSP through the UIF glue, hence the name used in the
> code.
>
> The module supports configuration of the CRC window through the crop
> rectangle on the ink pad of the corresponding entity. However, unlike

sink pad?

> the traditional V4L2 subdevice model, the crop rectangle does not
> influence the format on the source pad.
>
> Modeling the DISCOM as a sink-only entity would allow adhering to the
> V4L2 subdevice model at the expense of more complex code in the driver,
> as at the hardware level the UIF is handled as a sink+source entity. As
> the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it
> is not exposed to userspace through V4L2 but controlled through the DU
> driver. We can thus change this model later if needed without fear of
> affecting userspace.
>
> Signed-off-by: Laurent Pinchart 
> ---
> Changes since v1:
>
> - Don't return uninitialized value from uif_set_selection()
> ---
>  drivers/media/platform/vsp1/Makefile  |   2 +-
>  drivers/media/platform/vsp1/vsp1.h|   4 +
>  drivers/media/platform/vsp1/vsp1_drv.c|  20 +++
>  drivers/media/platform/vsp1/vsp1_entity.c |   6 +
>  drivers/media/platform/vsp1/vsp1_entity.h |   1 +
>  drivers/media/platform/vsp1/vsp1_regs.h   |  41 +
>  drivers/media/platform/vsp1/vsp1_uif.c| 271 
> ++
>  drivers/media/platform/vsp1/vsp1_uif.h|  32 
>  8 files changed, 376 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c
>  create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h
>
> diff --git a/drivers/media/platform/vsp1/Makefile 
> b/drivers/media/platform/vsp1/Makefile
> index 596775f932c0..4bb4dcbef7b5 100644
> --- a/drivers/media/platform/vsp1/Makefile
> +++ b/drivers/media/platform/vsp1/Makefile
> @@ -5,6 +5,6 @@ vsp1-y+= vsp1_rpf.o 
> vsp1_rwpf.o vsp1_wpf.o
>  vsp1-y   += vsp1_clu.o vsp1_hsit.o 
> vsp1_lut.o
>  vsp1-y   += vsp1_brx.o vsp1_sru.o 
> vsp1_uds.o
>  vsp1-y   += vsp1_hgo.o vsp1_hgt.o 
> vsp1_histo.o
> -vsp1-y   += vsp1_lif.o
> +vsp1-y   += vsp1_lif.o vsp1_uif.o
>
>  obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1.o
> diff --git a/drivers/media/platform/vsp1/vsp1.h 
> b/drivers/media/platform/vsp1/vsp1.h
> index 9cf4e1c4b036..33f632331474 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -36,10 +36,12 @@ struct vsp1_lut;
>  struct vsp1_rwpf;
>  struct vsp1_sru;
>  struct vsp1_uds;
> +struct vsp1_uif;
>
>  #define VSP1_MAX_LIF 2
>  #define VSP1_MAX_RPF 5
>  #define VSP1_MAX_UDS 3
> +#define VSP1_MAX_UIF 2
>  #define VSP1_MAX_WPF 4
>
>  #define VSP1_HAS_LUT (1 << 1)
> @@ -60,6 +62,7 @@ struct vsp1_device_info {
>   unsigned int lif_count;
>   unsigned int rpf_count;
>   unsigned int uds_count;
> + unsigned int uif_count;
>   unsigned int wpf_count;
>   unsigned int num_bru_inputs;
>   bool uapi;
> @@ -86,6 +89,7 @@ struct vsp1_device {
>   struct vsp1_rwpf *rpf[VSP1_MAX_RPF];
>   struct vsp1_sru *sru;
>   struct vsp1_uds *uds[VSP1_MAX_UDS];
> + struct vsp1_uif *uif[VSP1_MAX_UIF];
>   struct vsp1_rwpf *wpf[VSP1_MAX_WPF];
>
>   struct list_head entities;
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 331a2e0af0d3..d29f9c4baebe 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -35,6 +35,7 @@
>  #include "vsp1_rwpf.h"
>  #include "vsp1_sru.h"
>  #include "vsp1_uds.h"
> +#include "vsp1_uif.h"
>  #include "vsp1_video.h"
>
>  /* 
> -
> @@ -409,6 +410,19 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
>   list_add_tail(>entity.list_dev, >entities);
>   }
>
> + for (i = 0; i < vsp1->info->uif_count; ++i) {
> + struct vsp1_uif *uif;
> +
> + uif = vsp1_uif_create(vsp1, i);
> + if (IS_ERR(uif)) {
> + ret = PTR_ERR(uif);
> + goto done;
> + }
> +
> + vsp1->uif[i] = uif;
> + list_add_tail(>entity.list_dev, >entities);
> + }
> +
>   for (i = 0; i < vsp1->info->wpf_count; ++i) {
>   struct vsp1_rwpf *wpf;
>
> @@ -513,6 +527,9 @@ static int vsp1_device_init(struct vsp1_device *vsp1)
>   for (i = 0; i < vsp1->info->uds_count; ++i)
>   vsp1_write(vsp1, VI6_DPR_UDS_ROUTE(i),

[PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity

2018-04-22 Thread Laurent Pinchart

The DISCOM calculates a CRC on a configurable window of the frame. It
interfaces to the VSP through the UIF glue, hence the name used in the
code.

The module supports configuration of the CRC window through the crop
rectangle on the ink pad of the corresponding entity. However, unlike
the traditional V4L2 subdevice model, the crop rectangle does not
influence the format on the source pad.

Modeling the DISCOM as a sink-only entity would allow adhering to the
V4L2 subdevice model at the expense of more complex code in the driver,
as at the hardware level the UIF is handled as a sink+source entity. As
the DISCOM is only present in R-Car Gen3 VSP-D and VSP-DL instances it
is not exposed to userspace through V4L2 but controlled through the DU
driver. We can thus change this model later if needed without fear of
affecting userspace.

Signed-off-by: Laurent Pinchart 
---
Changes since v1:

- Don't return uninitialized value from uif_set_selection()
---
 drivers/media/platform/vsp1/Makefile  |   2 +-
 drivers/media/platform/vsp1/vsp1.h|   4 +
 drivers/media/platform/vsp1/vsp1_drv.c|  20 +++
 drivers/media/platform/vsp1/vsp1_entity.c |   6 +
 drivers/media/platform/vsp1/vsp1_entity.h |   1 +
 drivers/media/platform/vsp1/vsp1_regs.h   |  41 +
 drivers/media/platform/vsp1/vsp1_uif.c| 271 ++
 drivers/media/platform/vsp1/vsp1_uif.h|  32 
 8 files changed, 376 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/platform/vsp1/vsp1_uif.c
 create mode 100644 drivers/media/platform/vsp1/vsp1_uif.h

diff --git a/drivers/media/platform/vsp1/Makefile 
b/drivers/media/platform/vsp1/Makefile
index 596775f932c0..4bb4dcbef7b5 100644
--- a/drivers/media/platform/vsp1/Makefile
+++ b/drivers/media/platform/vsp1/Makefile
@@ -5,6 +5,6 @@ vsp1-y  += vsp1_rpf.o 
vsp1_rwpf.o vsp1_wpf.o
 vsp1-y += vsp1_clu.o vsp1_hsit.o vsp1_lut.o
 vsp1-y += vsp1_brx.o vsp1_sru.o vsp1_uds.o
 vsp1-y += vsp1_hgo.o vsp1_hgt.o vsp1_histo.o
-vsp1-y += vsp1_lif.o
+vsp1-y += vsp1_lif.o vsp1_uif.o
 
 obj-$(CONFIG_VIDEO_RENESAS_VSP1)   += vsp1.o
diff --git a/drivers/media/platform/vsp1/vsp1.h 
b/drivers/media/platform/vsp1/vsp1.h
index 9cf4e1c4b036..33f632331474 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -36,10 +36,12 @@ struct vsp1_lut;
 struct vsp1_rwpf;
 struct vsp1_sru;
 struct vsp1_uds;
+struct vsp1_uif;
 
 #define VSP1_MAX_LIF   2
 #define VSP1_MAX_RPF   5
 #define VSP1_MAX_UDS   3
+#define VSP1_MAX_UIF   2
 #define VSP1_MAX_WPF   4
 
 #define VSP1_HAS_LUT   (1 << 1)
@@ -60,6 +62,7 @@ struct vsp1_device_info {
unsigned int lif_count;
unsigned int rpf_count;
unsigned int uds_count;
+   unsigned int uif_count;
unsigned int wpf_count;
unsigned int num_bru_inputs;
bool uapi;
@@ -86,6 +89,7 @@ struct vsp1_device {
struct vsp1_rwpf *rpf[VSP1_MAX_RPF];
struct vsp1_sru *sru;
struct vsp1_uds *uds[VSP1_MAX_UDS];
+   struct vsp1_uif *uif[VSP1_MAX_UIF];
struct vsp1_rwpf *wpf[VSP1_MAX_WPF];
 
struct list_head entities;
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
b/drivers/media/platform/vsp1/vsp1_drv.c
index 331a2e0af0d3..d29f9c4baebe 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -35,6 +35,7 @@
 #include "vsp1_rwpf.h"
 #include "vsp1_sru.h"
 #include "vsp1_uds.h"
+#include "vsp1_uif.h"
 #include "vsp1_video.h"
 
 /* 
-
@@ -409,6 +410,19 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
list_add_tail(>entity.list_dev, >entities);
}
 
+   for (i = 0; i < vsp1->info->uif_count; ++i) {
+   struct vsp1_uif *uif;
+
+   uif = vsp1_uif_create(vsp1, i);
+   if (IS_ERR(uif)) {
+   ret = PTR_ERR(uif);
+   goto done;
+   }
+
+   vsp1->uif[i] = uif;
+   list_add_tail(>entity.list_dev, >entities);
+   }
+
for (i = 0; i < vsp1->info->wpf_count; ++i) {
struct vsp1_rwpf *wpf;
 
@@ -513,6 +527,9 @@ static int vsp1_device_init(struct vsp1_device *vsp1)
for (i = 0; i < vsp1->info->uds_count; ++i)
vsp1_write(vsp1, VI6_DPR_UDS_ROUTE(i), VI6_DPR_NODE_UNUSED);
 
+   for (i = 0; i < vsp1->info->uif_count; ++i)
+   vsp1_write(vsp1, VI6_DPR_UIF_ROUTE(i), VI6_DPR_NODE_UNUSED);
+
vsp1_write(vsp1, VI6_DPR_SRU_ROUTE, VI6_DPR_NODE_UNUSED);
vsp1_write(vsp1, VI6_DPR_LUT_ROUTE, VI6_DPR_NODE_UNUSED);
vsp1_write(vsp1, VI6_DPR_CLU_ROUTE,

Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity

Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity

Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity

[PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity

4 matches

Site Navigation

Mail list logo

Footer information