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
> > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > 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 000000000000..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 iirc.
Only when run with --strict.
Nevertheless, even if both styles are mixed in the driver, the predominant
style is !uif, so I'll switch to that.
> Those are very minor comments, so feel free to add my reviewed by tag
>
> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (soc_device_match(vsp1_r8a7796))
> > + uif->m3w_quirk = true;
> > +
> > + uif->entity.ops = &uif_entity_ops;
> > + uif->entity.type = VSP1_ENTITY_UIF;
> > + uif->entity.index = index;
> > +
> > + /* The datasheet names the two UIF instances UIF4 and UIF5. */
> > + sprintf(name, "uif.%u", index + 4);
> > + ret = vsp1_entity_init(vsp1, &uif->entity, name, 2, &uif_ops,
> > + MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> > + if (ret < 0)
> > + return ERR_PTR(ret);
> > +
> > + return uif;
> > +}
[snip]
--
Regards,
Laurent Pinchart
