Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity
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(&uds->entity.list_dev, &vsp1->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(&uif->entity.list_dev, &vsp1->entitie
Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity
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(&entity->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)
>
> Otherwise checkpatch complains i
Re: [PATCH v2 6/8] v4l: vsp1: Add support for the DISCOM entity
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(&uds->entity.list_dev, &vsp1->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(&uif->entity.list_dev, &vsp1->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);
>
>
