| Message ID | 1473808626-19488-7-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 2016-09-14 02:16:59 +0300, Laurent Pinchart wrote: > Cropping on the WPF sink pad restricts the left and top coordinates to > 0-255. The same result can be obtained by cropping on the RPF without > any such restriction, this feature isn't useful. Disable it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/vsp1/vsp1_rwpf.c | 37 +++++++++++++++++---------------- > drivers/media/platform/vsp1/vsp1_wpf.c | 18 +++++++--------- > 2 files changed, 26 insertions(+), 29 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c > index 8cb87e96b78b..a3ace8df7f4d 100644 > --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c > @@ -66,7 +66,6 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev, > struct vsp1_rwpf *rwpf = to_rwpf(subdev); > struct v4l2_subdev_pad_config *config; > struct v4l2_mbus_framefmt *format; > - struct v4l2_rect *crop; > int ret = 0; > > mutex_lock(&rwpf->entity.lock); > @@ -103,12 +102,16 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev, > > fmt->format = *format; > > - /* Update the sink crop rectangle. */ > - crop = vsp1_rwpf_get_crop(rwpf, config); > - crop->left = 0; > - crop->top = 0; > - crop->width = fmt->format.width; > - crop->height = fmt->format.height; > + if (rwpf->entity.type == VSP1_ENTITY_RPF) { > + struct v4l2_rect *crop; > + > + /* Update the sink crop rectangle. */ > + crop = vsp1_rwpf_get_crop(rwpf, config); > + crop->left = 0; > + crop->top = 0; > + crop->width = fmt->format.width; > + crop->height = fmt->format.height; > + } > > /* Propagate the format to the source pad. */ > format = vsp1_entity_get_pad_format(&rwpf->entity, config, > @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct v4l2_subdev *subdev, > struct v4l2_mbus_framefmt *format; > int ret = 0; > > - /* Cropping is implemented on the sink pad. */ > - if (sel->pad != RWPF_PAD_SINK) > + /* Cropping is only supported on the RPF and is implemented on the sink > + * pad. > + */ > + if (rwpf->entity.type == VSP1_ENTITY_WPF || sel->pad != RWPF_PAD_SINK) > return -EINVAL; > > mutex_lock(&rwpf->entity.lock); > @@ -175,8 +180,10 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, > struct v4l2_rect *crop; > int ret = 0; > > - /* Cropping is implemented on the sink pad. */ > - if (sel->pad != RWPF_PAD_SINK) > + /* Cropping is only supported on the RPF and is implemented on the sink > + * pad. > + */ > + if (rwpf->entity.type == VSP1_ENTITY_WPF || sel->pad != RWPF_PAD_SINK) > return -EINVAL; > > if (sel->target != V4L2_SEL_TGT_CROP) > @@ -190,9 +197,7 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, > goto done; > } > > - /* Make sure the crop rectangle is entirely contained in the image. The > - * WPF top and left offsets are limited to 255. > - */ > + /* Make sure the crop rectangle is entirely contained in the image. */ > format = vsp1_entity_get_pad_format(&rwpf->entity, config, > RWPF_PAD_SINK); > > @@ -208,10 +213,6 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, > > sel->r.left = min_t(unsigned int, sel->r.left, format->width - 2); > sel->r.top = min_t(unsigned int, sel->r.top, format->height - 2); > - if (rwpf->entity.type == VSP1_ENTITY_WPF) { > - sel->r.left = min_t(unsigned int, sel->r.left, 255); > - sel->r.top = min_t(unsigned int, sel->r.top, 255); > - } > sel->r.width = min_t(unsigned int, sel->r.width, > format->width - sel->r.left); > sel->r.height = min_t(unsigned int, sel->r.height, > diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c > index 748f5af90b7e..f3a593196282 100644 > --- a/drivers/media/platform/vsp1/vsp1_wpf.c > +++ b/drivers/media/platform/vsp1/vsp1_wpf.c > @@ -212,7 +212,6 @@ static void wpf_configure(struct vsp1_entity *entity, > struct vsp1_device *vsp1 = wpf->entity.vsp1; > const struct v4l2_mbus_framefmt *source_format; > const struct v4l2_mbus_framefmt *sink_format; > - const struct v4l2_rect *crop; > unsigned int i; > u32 outfmt = 0; > u32 srcrpf = 0; > @@ -237,16 +236,6 @@ static void wpf_configure(struct vsp1_entity *entity, > return; > } > > - /* Cropping */ > - crop = vsp1_rwpf_get_crop(wpf, wpf->entity.config); > - > - vsp1_wpf_write(wpf, dl, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN | > - (crop->left << VI6_WPF_SZCLIP_OFST_SHIFT) | > - (crop->width << VI6_WPF_SZCLIP_SIZE_SHIFT)); > - vsp1_wpf_write(wpf, dl, VI6_WPF_VSZCLIP, VI6_WPF_SZCLIP_EN | > - (crop->top << VI6_WPF_SZCLIP_OFST_SHIFT) | > - (crop->height << VI6_WPF_SZCLIP_SIZE_SHIFT)); > - > /* Format */ > sink_format = vsp1_entity_get_pad_format(&wpf->entity, > wpf->entity.config, > @@ -255,6 +244,13 @@ static void wpf_configure(struct vsp1_entity *entity, > wpf->entity.config, > RWPF_PAD_SOURCE); > > + vsp1_wpf_write(wpf, dl, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN | > + (0 << VI6_WPF_SZCLIP_OFST_SHIFT) | > + (source_format->width << VI6_WPF_SZCLIP_SIZE_SHIFT)); > + vsp1_wpf_write(wpf, dl, VI6_WPF_VSZCLIP, VI6_WPF_SZCLIP_EN | > + (0 << VI6_WPF_SZCLIP_OFST_SHIFT) | > + (source_format->height << VI6_WPF_SZCLIP_SIZE_SHIFT)); > + > if (!pipe->lif) { > const struct v4l2_pix_format_mplane *format = &wpf->format; > const struct vsp1_format_info *fmtinfo = wpf->fmtinfo; > -- > Regards, > > Laurent Pinchart >
Em Wed, 14 Sep 2016 02:16:59 +0300 Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> escreveu: > Cropping on the WPF sink pad restricts the left and top coordinates to > 0-255. The same result can be obtained by cropping on the RPF without > any such restriction, this feature isn't useful. Disable it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/platform/vsp1/vsp1_rwpf.c | 37 +++++++++++++++++---------------- > drivers/media/platform/vsp1/vsp1_wpf.c | 18 +++++++--------- > 2 files changed, 26 insertions(+), 29 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c > index 8cb87e96b78b..a3ace8df7f4d 100644 > --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c > @@ -66,7 +66,6 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev, > struct vsp1_rwpf *rwpf = to_rwpf(subdev); > struct v4l2_subdev_pad_config *config; > struct v4l2_mbus_framefmt *format; > - struct v4l2_rect *crop; > int ret = 0; > > mutex_lock(&rwpf->entity.lock); > @@ -103,12 +102,16 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev, > > fmt->format = *format; > > - /* Update the sink crop rectangle. */ > - crop = vsp1_rwpf_get_crop(rwpf, config); > - crop->left = 0; > - crop->top = 0; > - crop->width = fmt->format.width; > - crop->height = fmt->format.height; > + if (rwpf->entity.type == VSP1_ENTITY_RPF) { > + struct v4l2_rect *crop; > + > + /* Update the sink crop rectangle. */ > + crop = vsp1_rwpf_get_crop(rwpf, config); > + crop->left = 0; > + crop->top = 0; > + crop->width = fmt->format.width; > + crop->height = fmt->format.height; > + } > > /* Propagate the format to the source pad. */ > format = vsp1_entity_get_pad_format(&rwpf->entity, config, > @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct v4l2_subdev *subdev, > struct v4l2_mbus_framefmt *format; > int ret = 0; > > - /* Cropping is implemented on the sink pad. */ > - if (sel->pad != RWPF_PAD_SINK) > + /* Cropping is only supported on the RPF and is implemented on the sink > + * pad. > + */ Please read CodingStyle and run checkpatch before sending stuff upstream. This violates the CodingStyle: it should be, instead: /* * foo * bar */ This time, I'll fix it, but next time I might not have enough time, and need to reject the patch series. Thanks, Mauro
Hi Mauro, On Monday 19 Sep 2016 14:55:43 Mauro Carvalho Chehab wrote: > Em Wed, 14 Sep 2016 02:16:59 +0300 Laurent Pinchart escreveu: > > Cropping on the WPF sink pad restricts the left and top coordinates to > > 0-255. The same result can be obtained by cropping on the RPF without > > any such restriction, this feature isn't useful. Disable it. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > drivers/media/platform/vsp1/vsp1_rwpf.c | 37 > > +++++++++++++++++---------------- drivers/media/platform/vsp1/vsp1_wpf.c > > | 18 +++++++--------- > > 2 files changed, 26 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c > > b/drivers/media/platform/vsp1/vsp1_rwpf.c index > > 8cb87e96b78b..a3ace8df7f4d 100644 > > --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c > > @@ -66,7 +66,6 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev > > *subdev,> > > struct vsp1_rwpf *rwpf = to_rwpf(subdev); > > struct v4l2_subdev_pad_config *config; > > struct v4l2_mbus_framefmt *format; > > > > - struct v4l2_rect *crop; > > > > int ret = 0; > > > > mutex_lock(&rwpf->entity.lock); > > > > @@ -103,12 +102,16 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev > > *subdev,> > > fmt->format = *format; > > > > - /* Update the sink crop rectangle. */ > > - crop = vsp1_rwpf_get_crop(rwpf, config); > > - crop->left = 0; > > - crop->top = 0; > > - crop->width = fmt->format.width; > > - crop->height = fmt->format.height; > > + if (rwpf->entity.type == VSP1_ENTITY_RPF) { > > + struct v4l2_rect *crop; > > + > > + /* Update the sink crop rectangle. */ > > + crop = vsp1_rwpf_get_crop(rwpf, config); > > + crop->left = 0; > > + crop->top = 0; > > + crop->width = fmt->format.width; > > + crop->height = fmt->format.height; > > + } > > > > /* Propagate the format to the source pad. */ > > format = vsp1_entity_get_pad_format(&rwpf->entity, config, > > > > @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct v4l2_subdev > > *subdev,> > > struct v4l2_mbus_framefmt *format; > > int ret = 0; > > > > - /* Cropping is implemented on the sink pad. */ > > - if (sel->pad != RWPF_PAD_SINK) > > + /* Cropping is only supported on the RPF and is implemented on the sink > > + * pad. > > + */ > > Please read CodingStyle and run checkpatch before sending stuff upstream. > > This violates the CodingStyle: it should be, instead: > /* > * foo > * bar > */ But it's consistent with the coding style of this driver. I'm OK fixing it, but it should be done globally in that case. > This time, I'll fix it, but next time I might not have enough time, and > need to reject the patch series.
Em Mon, 19 Sep 2016 20:59:56 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > On Monday 19 Sep 2016 14:55:43 Mauro Carvalho Chehab wrote: > > Em Wed, 14 Sep 2016 02:16:59 +0300 Laurent Pinchart escreveu: > > > Cropping on the WPF sink pad restricts the left and top coordinates to > > > 0-255. The same result can be obtained by cropping on the RPF without > > > any such restriction, this feature isn't useful. Disable it. > > > > > > Signed-off-by: Laurent Pinchart > > > <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > > > > drivers/media/platform/vsp1/vsp1_rwpf.c | 37 > > > +++++++++++++++++---------------- drivers/media/platform/vsp1/vsp1_wpf.c > > > | 18 +++++++--------- > > > 2 files changed, 26 insertions(+), 29 deletions(-) > > > > > > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c > > > b/drivers/media/platform/vsp1/vsp1_rwpf.c index > > > 8cb87e96b78b..a3ace8df7f4d 100644 > > > --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > > > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c > > > @@ -66,7 +66,6 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev > > > *subdev,> > > > struct vsp1_rwpf *rwpf = to_rwpf(subdev); > > > struct v4l2_subdev_pad_config *config; > > > struct v4l2_mbus_framefmt *format; > > > > > > - struct v4l2_rect *crop; > > > > > > int ret = 0; > > > > > > mutex_lock(&rwpf->entity.lock); > > > > > > @@ -103,12 +102,16 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev > > > *subdev,> > > > fmt->format = *format; > > > > > > - /* Update the sink crop rectangle. */ > > > - crop = vsp1_rwpf_get_crop(rwpf, config); > > > - crop->left = 0; > > > - crop->top = 0; > > > - crop->width = fmt->format.width; > > > - crop->height = fmt->format.height; > > > + if (rwpf->entity.type == VSP1_ENTITY_RPF) { > > > + struct v4l2_rect *crop; > > > + > > > + /* Update the sink crop rectangle. */ > > > + crop = vsp1_rwpf_get_crop(rwpf, config); > > > + crop->left = 0; > > > + crop->top = 0; > > > + crop->width = fmt->format.width; > > > + crop->height = fmt->format.height; > > > + } > > > > > > /* Propagate the format to the source pad. */ > > > format = vsp1_entity_get_pad_format(&rwpf->entity, config, > > > > > > @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct v4l2_subdev > > > *subdev,> > > > struct v4l2_mbus_framefmt *format; > > > int ret = 0; > > > > > > - /* Cropping is implemented on the sink pad. */ > > > - if (sel->pad != RWPF_PAD_SINK) > > > + /* Cropping is only supported on the RPF and is implemented on the > sink > > > + * pad. > > > + */ > > > > Please read CodingStyle and run checkpatch before sending stuff upstream. > > > > This violates the CodingStyle: it should be, instead: > > /* > > * foo > > * bar > > */ > > But it's consistent with the coding style of this driver. I'm OK fixing it, > but it should be done globally in that case. There are inconsistencies inside the driver too on multi-line comments even without fixing the ones introduced on this series, as, on several places, multi-line comments are correct: drivers/media/platform/vsp1/vsp1_bru.c:/* drivers/media/platform/vsp1/vsp1_bru.c- * The BRU can't perform format conversion, all sink and source formats must be drivers/media/platform/vsp1/vsp1_bru.c- * identical. We pick the format on the first sink pad (pad 0) and propagate it drivers/media/platform/vsp1/vsp1_bru.c- * to all other pads. drivers/media/platform/vsp1/vsp1_bru.c- */ drivers/media/platform/vsp1/vsp1_dl.c:/* drivers/media/platform/vsp1/vsp1_dl.c- * Initialize a display list body object and allocate DMA memory for the body drivers/media/platform/vsp1/vsp1_dl.c- * data. The display list body object is expected to have been initialized to drivers/media/platform/vsp1/vsp1_dl.c- * 0 when allocated. drivers/media/platform/vsp1/vsp1_dl.c- */ ... I'll address the ones only the CodingStyle violation introduced by this series. I'll leave for the vsp1 maintainers/developers. Btw, there are several kernel-doc tags that use just: /* ... */ instead of: /** ... */ I suggest you to add the files/headers with kernel-doc markups on a Documentation/media/v4l-drivers/vsp1.rst file, to be created. This way, you can validate that such documentation is correct, and produce an auto-generated documentation for this driver. Regards, Mauro
Hi Mauro, On Monday 19 Sep 2016 15:26:15 Mauro Carvalho Chehab wrote: > Em Mon, 19 Sep 2016 20:59:56 +0300 Laurent Pinchart escreveu: > > On Monday 19 Sep 2016 14:55:43 Mauro Carvalho Chehab wrote: > >> Em Wed, 14 Sep 2016 02:16:59 +0300 Laurent Pinchart escreveu: > >>> Cropping on the WPF sink pad restricts the left and top coordinates to > >>> 0-255. The same result can be obtained by cropping on the RPF without > >>> any such restriction, this feature isn't useful. Disable it. > >>> > >>> Signed-off-by: Laurent Pinchart > >>> <laurent.pinchart+renesas@ideasonboard.com> > >>> --- > >>> > >>> drivers/media/platform/vsp1/vsp1_rwpf.c | 37 ++++++++++++------------ > >>> drivers/media/platform/vsp1/vsp1_wpf.c | 18 +++++++--------- > >>> 2 files changed, 26 insertions(+), 29 deletions(-) > >>> > >>> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c > >>> b/drivers/media/platform/vsp1/vsp1_rwpf.c index > >>> 8cb87e96b78b..a3ace8df7f4d 100644 > >>> --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > >>> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c [snip] > >>> @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct > >>> v4l2_subdev *subdev, > >>> struct v4l2_mbus_framefmt *format; > >>> int ret = 0; > >>> > >>> - /* Cropping is implemented on the sink pad. */ > >>> - if (sel->pad != RWPF_PAD_SINK) > >>> + /* Cropping is only supported on the RPF and is implemented on > >>> the sink > >>> + * pad. > >>> + */ > >> > >> Please read CodingStyle and run checkpatch before sending stuff > >> upstream. > >> > >> This violates the CodingStyle: it should be, instead: > >> /* > >> * foo > >> * bar > >> */ > > > > But it's consistent with the coding style of this driver. I'm OK fixing > > it, but it should be done globally in that case. > > There are inconsistencies inside the driver too on multi-line > comments even without fixing the ones introduced on this series, > as, on several places, multi-line comments are correct: > > drivers/media/platform/vsp1/vsp1_bru.c:/* > drivers/media/platform/vsp1/vsp1_bru.c- * The BRU can't perform format > conversion, all sink and source formats must be > drivers/media/platform/vsp1/vsp1_bru.c- * identical. We pick the format on > the first sink pad (pad 0) and propagate it > drivers/media/platform/vsp1/vsp1_bru.c- * to all other pads. > drivers/media/platform/vsp1/vsp1_bru.c- */ > > drivers/media/platform/vsp1/vsp1_dl.c:/* > drivers/media/platform/vsp1/vsp1_dl.c- * Initialize a display list body > object and allocate DMA memory for the body > drivers/media/platform/vsp1/vsp1_dl.c- * data. The display list body object > is expected to have been initialized to > drivers/media/platform/vsp1/vsp1_dl.c- * 0 when allocated. > drivers/media/platform/vsp1/vsp1_dl.c- */ > > ... > > I'll address the ones only the CodingStyle violation introduced by this > series. I'll leave for the vsp1 maintainers/developers. OK, I'll address that. > Btw, there are several kernel-doc tags that use just: > /* > ... > */ > > instead of: > > /** > ... > */ > > I suggest you to add the files/headers with kernel-doc markups on > a Documentation/media/v4l-drivers/vsp1.rst file, to be created. > > This way, you can validate that such documentation is correct, > and produce an auto-generated documentation for this driver. I've thought about it, but I don't think those comments should become part of the kernel documentation. They're really about driver internals, and meant for the driver developers. In particular only a subset of the driver is documented that way, when I've considered that the code or structures were complex enough to need proper documentation. A generated doc would then be quite incomplete and not very useful, the comments are meant to be read while working on the code.
Em Mon, 19 Sep 2016 21:33:13 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > On Monday 19 Sep 2016 15:26:15 Mauro Carvalho Chehab wrote: > > Em Mon, 19 Sep 2016 20:59:56 +0300 Laurent Pinchart escreveu: > > > On Monday 19 Sep 2016 14:55:43 Mauro Carvalho Chehab wrote: > > >> Em Wed, 14 Sep 2016 02:16:59 +0300 Laurent Pinchart escreveu: > > >>> Cropping on the WPF sink pad restricts the left and top coordinates to > > >>> 0-255. The same result can be obtained by cropping on the RPF without > > >>> any such restriction, this feature isn't useful. Disable it. > > >>> > > >>> Signed-off-by: Laurent Pinchart > > >>> <laurent.pinchart+renesas@ideasonboard.com> > > >>> --- > > >>> > > >>> drivers/media/platform/vsp1/vsp1_rwpf.c | 37 ++++++++++++------------ > > >>> drivers/media/platform/vsp1/vsp1_wpf.c | 18 +++++++--------- > > >>> 2 files changed, 26 insertions(+), 29 deletions(-) > > >>> > > >>> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c > > >>> b/drivers/media/platform/vsp1/vsp1_rwpf.c index > > >>> 8cb87e96b78b..a3ace8df7f4d 100644 > > >>> --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > > >>> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c > > [snip] > > > >>> @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct > > >>> v4l2_subdev *subdev, > > >>> struct v4l2_mbus_framefmt *format; > > >>> int ret = 0; > > >>> > > >>> - /* Cropping is implemented on the sink pad. */ > > >>> - if (sel->pad != RWPF_PAD_SINK) > > >>> + /* Cropping is only supported on the RPF and is implemented on > > >>> the sink > > >>> + * pad. > > >>> + */ > > >> > > >> Please read CodingStyle and run checkpatch before sending stuff > > >> upstream. > > >> > > >> This violates the CodingStyle: it should be, instead: > > >> /* > > >> * foo > > >> * bar > > >> */ > > > > > > But it's consistent with the coding style of this driver. I'm OK fixing > > > it, but it should be done globally in that case. > > > > There are inconsistencies inside the driver too on multi-line > > comments even without fixing the ones introduced on this series, > > as, on several places, multi-line comments are correct: > > > > drivers/media/platform/vsp1/vsp1_bru.c:/* > > drivers/media/platform/vsp1/vsp1_bru.c- * The BRU can't perform format > > conversion, all sink and source formats must be > > drivers/media/platform/vsp1/vsp1_bru.c- * identical. We pick the format on > > the first sink pad (pad 0) and propagate it > > drivers/media/platform/vsp1/vsp1_bru.c- * to all other pads. > > drivers/media/platform/vsp1/vsp1_bru.c- */ > > > > drivers/media/platform/vsp1/vsp1_dl.c:/* > > drivers/media/platform/vsp1/vsp1_dl.c- * Initialize a display list body > > object and allocate DMA memory for the body > > drivers/media/platform/vsp1/vsp1_dl.c- * data. The display list body object > > is expected to have been initialized to > > drivers/media/platform/vsp1/vsp1_dl.c- * 0 when allocated. > > drivers/media/platform/vsp1/vsp1_dl.c- */ > > > > ... > > > > I'll address the ones only the CodingStyle violation introduced by this > > series. I'll leave for the vsp1 maintainers/developers. > > OK, I'll address that. > > > Btw, there are several kernel-doc tags that use just: > > /* > > ... > > */ > > > > instead of: > > > > /** > > ... > > */ > > > > I suggest you to add the files/headers with kernel-doc markups on > > a Documentation/media/v4l-drivers/vsp1.rst file, to be created. > > > > This way, you can validate that such documentation is correct, > > and produce an auto-generated documentation for this driver. > > I've thought about it, but I don't think those comments should become part of > the kernel documentation. They're really about driver internals, and meant for > the driver developers. In particular only a subset of the driver is documented > that way, when I've considered that the code or structures were complex enough > to need proper documentation. A generated doc would then be quite incomplete > and not very useful, the comments are meant to be read while working on the > code. The v4l-drivers book is meant to have driver internals documentation, and not the subsystem kAPI or uAPI. I don't see any problems if you want to document just the more complex functions/structs over the v4l-drivers/ book. Yet, as you already took the time to write documentation for those functions, providing that the kernel-doc markups are ok, a v4l-drivers/vsp1.rst file for it could be as simple as (untested): VSP1 driver ^^^^^^^^^^^ .. kernel-doc:: drivers/media/platform/vsp1/vsp1_dl.c .. kernel-doc:: drivers/media/platform/vsp1/vsp1_drm.c .. kernel-doc:: drivers/media/platform/vsp1/vsp1_drm.h .. kernel-doc:: drivers/media/platform/vsp1/vsp1_entity.c .. kernel-doc:: drivers/media/platform/vsp1/vsp1_entity.h .. kernel-doc:: drivers/media/platform/vsp1/vsp1_pipe.c .. kernel-doc:: drivers/media/platform/vsp1/vsp1_video.c PS.: Eventually, you may need an extra attribute for the files with EXPORT_SYMBOL*, in order to associate a *.c file with the corresponding *.h file.
diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c index 8cb87e96b78b..a3ace8df7f4d 100644 --- a/drivers/media/platform/vsp1/vsp1_rwpf.c +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c @@ -66,7 +66,6 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev, struct vsp1_rwpf *rwpf = to_rwpf(subdev); struct v4l2_subdev_pad_config *config; struct v4l2_mbus_framefmt *format; - struct v4l2_rect *crop; int ret = 0; mutex_lock(&rwpf->entity.lock); @@ -103,12 +102,16 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev, fmt->format = *format; - /* Update the sink crop rectangle. */ - crop = vsp1_rwpf_get_crop(rwpf, config); - crop->left = 0; - crop->top = 0; - crop->width = fmt->format.width; - crop->height = fmt->format.height; + if (rwpf->entity.type == VSP1_ENTITY_RPF) { + struct v4l2_rect *crop; + + /* Update the sink crop rectangle. */ + crop = vsp1_rwpf_get_crop(rwpf, config); + crop->left = 0; + crop->top = 0; + crop->width = fmt->format.width; + crop->height = fmt->format.height; + } /* Propagate the format to the source pad. */ format = vsp1_entity_get_pad_format(&rwpf->entity, config, @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct v4l2_subdev *subdev, struct v4l2_mbus_framefmt *format; int ret = 0; - /* Cropping is implemented on the sink pad. */ - if (sel->pad != RWPF_PAD_SINK) + /* Cropping is only supported on the RPF and is implemented on the sink + * pad. + */ + if (rwpf->entity.type == VSP1_ENTITY_WPF || sel->pad != RWPF_PAD_SINK) return -EINVAL; mutex_lock(&rwpf->entity.lock); @@ -175,8 +180,10 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, struct v4l2_rect *crop; int ret = 0; - /* Cropping is implemented on the sink pad. */ - if (sel->pad != RWPF_PAD_SINK) + /* Cropping is only supported on the RPF and is implemented on the sink + * pad. + */ + if (rwpf->entity.type == VSP1_ENTITY_WPF || sel->pad != RWPF_PAD_SINK) return -EINVAL; if (sel->target != V4L2_SEL_TGT_CROP) @@ -190,9 +197,7 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, goto done; } - /* Make sure the crop rectangle is entirely contained in the image. The - * WPF top and left offsets are limited to 255. - */ + /* Make sure the crop rectangle is entirely contained in the image. */ format = vsp1_entity_get_pad_format(&rwpf->entity, config, RWPF_PAD_SINK); @@ -208,10 +213,6 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, sel->r.left = min_t(unsigned int, sel->r.left, format->width - 2); sel->r.top = min_t(unsigned int, sel->r.top, format->height - 2); - if (rwpf->entity.type == VSP1_ENTITY_WPF) { - sel->r.left = min_t(unsigned int, sel->r.left, 255); - sel->r.top = min_t(unsigned int, sel->r.top, 255); - } sel->r.width = min_t(unsigned int, sel->r.width, format->width - sel->r.left); sel->r.height = min_t(unsigned int, sel->r.height, diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c index 748f5af90b7e..f3a593196282 100644 --- a/drivers/media/platform/vsp1/vsp1_wpf.c +++ b/drivers/media/platform/vsp1/vsp1_wpf.c @@ -212,7 +212,6 @@ static void wpf_configure(struct vsp1_entity *entity, struct vsp1_device *vsp1 = wpf->entity.vsp1; const struct v4l2_mbus_framefmt *source_format; const struct v4l2_mbus_framefmt *sink_format; - const struct v4l2_rect *crop; unsigned int i; u32 outfmt = 0; u32 srcrpf = 0; @@ -237,16 +236,6 @@ static void wpf_configure(struct vsp1_entity *entity, return; } - /* Cropping */ - crop = vsp1_rwpf_get_crop(wpf, wpf->entity.config); - - vsp1_wpf_write(wpf, dl, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN | - (crop->left << VI6_WPF_SZCLIP_OFST_SHIFT) | - (crop->width << VI6_WPF_SZCLIP_SIZE_SHIFT)); - vsp1_wpf_write(wpf, dl, VI6_WPF_VSZCLIP, VI6_WPF_SZCLIP_EN | - (crop->top << VI6_WPF_SZCLIP_OFST_SHIFT) | - (crop->height << VI6_WPF_SZCLIP_SIZE_SHIFT)); - /* Format */ sink_format = vsp1_entity_get_pad_format(&wpf->entity, wpf->entity.config, @@ -255,6 +244,13 @@ static void wpf_configure(struct vsp1_entity *entity, wpf->entity.config, RWPF_PAD_SOURCE); + vsp1_wpf_write(wpf, dl, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN | + (0 << VI6_WPF_SZCLIP_OFST_SHIFT) | + (source_format->width << VI6_WPF_SZCLIP_SIZE_SHIFT)); + vsp1_wpf_write(wpf, dl, VI6_WPF_VSZCLIP, VI6_WPF_SZCLIP_EN | + (0 << VI6_WPF_SZCLIP_OFST_SHIFT) | + (source_format->height << VI6_WPF_SZCLIP_SIZE_SHIFT)); + if (!pipe->lif) { const struct v4l2_pix_format_mplane *format = &wpf->format; const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
Cropping on the WPF sink pad restricts the left and top coordinates to 0-255. The same result can be obtained by cropping on the RPF without any such restriction, this feature isn't useful. Disable it. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/media/platform/vsp1/vsp1_rwpf.c | 37 +++++++++++++++++---------------- drivers/media/platform/vsp1/vsp1_wpf.c | 18 +++++++--------- 2 files changed, 26 insertions(+), 29 deletions(-)