| Message ID | 20231024142522.29658-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vsp1: Remove unbalanced .s_stream(0) calls | expand |
Hi Laurent, On Tue, Oct 24, 2023 at 4:25 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > The VSP1 driver uses the subdev .s_stream() operation to stop WPF > instances, without a corresponding call to start them. The V4L2 subdev > core started warning about unbalanced .s_stream() calls in commit > 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() > requirements"), causing a regression with this driver. > > Fix the problem by replacing the .s_stream() operation with an explicit > function call for WPF instances. This allows sharing an additional data > structure between RPF and WPF instances. > > Fixes: 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() requirements") > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Closes: https://lore.kernel.org/linux-media/2221395-6a9b-9527-d697-e76aebc6af@linux-m68k.org/ > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thanks for your patch! The warning splat is gone, so Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Laurent, On Tue, Oct 24, 2023 at 4:56 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Oct 24, 2023 at 4:25 PM Laurent Pinchart > <laurent.pinchart+renesas@ideasonboard.com> wrote: > > The VSP1 driver uses the subdev .s_stream() operation to stop WPF > > instances, without a corresponding call to start them. The V4L2 subdev > > core started warning about unbalanced .s_stream() calls in commit > > 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() > > requirements"), causing a regression with this driver. > > > > Fix the problem by replacing the .s_stream() operation with an explicit > > function call for WPF instances. This allows sharing an additional data > > structure between RPF and WPF instances. > > > > Fixes: 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() requirements") > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Closes: https://lore.kernel.org/linux-media/2221395-6a9b-9527-d697-e76aebc6af@linux-m68k.org/ > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Thanks for your patch! > > The warning splat is gone, so > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> FTR, the warning splat is now in v6.7-rc1, but the fix is not (not even in linux-next). Gr{oetje,eeting}s, Geert
Hi Geert, On Mon, Nov 13, 2023 at 03:05:07PM +0100, Geert Uytterhoeven wrote: > On Tue, Oct 24, 2023 at 4:56 PM Geert Uytterhoeven wrote: > > On Tue, Oct 24, 2023 at 4:25 PM Laurent Pinchart wrote: > > > The VSP1 driver uses the subdev .s_stream() operation to stop WPF > > > instances, without a corresponding call to start them. The V4L2 subdev > > > core started warning about unbalanced .s_stream() calls in commit > > > 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() > > > requirements"), causing a regression with this driver. > > > > > > Fix the problem by replacing the .s_stream() operation with an explicit > > > function call for WPF instances. This allows sharing an additional data > > > structure between RPF and WPF instances. > > > > > > Fixes: 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() requirements") > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > Closes: https://lore.kernel.org/linux-media/2221395-6a9b-9527-d697-e76aebc6af@linux-m68k.org/ > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > > Thanks for your patch! > > > > The warning splat is gone, so > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > FTR, the warning splat is now in v6.7-rc1, but the fix is not > (not even in linux-next). I know. I've sent a pull request for it yesterday, it should get merged in time for v6.7.
Hi Laurent, Thanks for the patch. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Sent: Tuesday, October 24, 2023 3:25 PM > To: linux-media@vger.kernel.org > Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>; linux-renesas- > soc@vger.kernel.org; Geert Uytterhoeven <geert@linux-m68k.org> > Subject: [PATCH] media: vsp1: Remove unbalanced .s_stream(0) calls > > The VSP1 driver uses the subdev .s_stream() operation to stop WPF > instances, without a corresponding call to start them. The V4L2 subdev > core started warning about unbalanced .s_stream() calls in commit > 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() > requirements"), causing a regression with this driver. > > Fix the problem by replacing the .s_stream() operation with an explicit > function call for WPF instances. This allows sharing an additional data > structure between RPF and WPF instances. > > Fixes: 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() > requirements") > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Closes: > Signed-off-by: Laurent Pinchart > <laurent.pinchart+renesas@ideasonboard.com> Unbind/ bind calls works fine on RZ/G2L DRM driver with this patch. Tested-by: Biju Das <biju.das.jz@bp.renesas.com> Cheers, Biju > --- > .../media/platform/renesas/vsp1/vsp1_pipe.c | 2 +- > .../media/platform/renesas/vsp1/vsp1_rpf.c | 10 +------ > .../media/platform/renesas/vsp1/vsp1_rwpf.c | 8 +++-- > .../media/platform/renesas/vsp1/vsp1_rwpf.h | 4 ++- > .../media/platform/renesas/vsp1/vsp1_wpf.c | 29 ++----------------- > 5 files changed, 14 insertions(+), 39 deletions(-) > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c > b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c > index f8093ba9539e..68d05243c3ee 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c > @@ -373,7 +373,7 @@ int vsp1_pipeline_stop(struct vsp1_pipeline *pipe) > (7 << VI6_DPR_SMPPT_TGW_SHIFT) | > (VI6_DPR_NODE_UNUSED << VI6_DPR_SMPPT_PT_SHIFT)); > > - v4l2_subdev_call(&pipe->output->entity.subdev, video, s_stream, 0); > + vsp1_wpf_stop(pipe->output); > > return ret; > } > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c > b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c > index 3b17f5fa4067..ea12c3f12c92 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c > @@ -43,14 +43,6 @@ static inline void vsp1_rpf_write(struct vsp1_rwpf > *rpf, > data); > } > > -/* ---------------------------------------------------------------------- > ------- > - * V4L2 Subdevice Operations > - */ > - > -static const struct v4l2_subdev_ops rpf_ops = { > - .pad = &vsp1_rwpf_pad_ops, > -}; > - > /* ---------------------------------------------------------------------- > ------- > * VSP1 Entity Operations > */ > @@ -411,7 +403,7 @@ struct vsp1_rwpf *vsp1_rpf_create(struct vsp1_device > *vsp1, unsigned int index) > rpf->entity.index = index; > > sprintf(name, "rpf.%u", index); > - ret = vsp1_entity_init(vsp1, &rpf->entity, name, 2, &rpf_ops, > + ret = vsp1_entity_init(vsp1, &rpf->entity, name, 2, > +&vsp1_rwpf_subdev_ops, > MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER); > if (ret < 0) > return ERR_PTR(ret); > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c > b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c > index 22a82d218152..e0f87c8103ca 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c > @@ -24,7 +24,7 @@ struct v4l2_rect *vsp1_rwpf_get_crop(struct vsp1_rwpf > *rwpf, } > > /* ---------------------------------------------------------------------- > ------- > - * V4L2 Subdevice Pad Operations > + * V4L2 Subdevice Operations > */ > > static int vsp1_rwpf_enum_mbus_code(struct v4l2_subdev *subdev, @@ -243,7 > +243,7 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, > return ret; > } > > -const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops = { > +static const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops = { > .init_cfg = vsp1_entity_init_cfg, > .enum_mbus_code = vsp1_rwpf_enum_mbus_code, > .enum_frame_size = vsp1_rwpf_enum_frame_size, @@ -253,6 +253,10 @@ > const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops = { > .set_selection = vsp1_rwpf_set_selection, }; > > +const struct v4l2_subdev_ops vsp1_rwpf_subdev_ops = { > + .pad = &vsp1_rwpf_pad_ops, > +}; > + > /* ---------------------------------------------------------------------- > ------- > * Controls > */ > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.h > b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.h > index eac5c04c2239..e0d212c70b2f 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.h > +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.h > @@ -79,9 +79,11 @@ static inline struct vsp1_rwpf *entity_to_rwpf(struct > vsp1_entity *entity) struct vsp1_rwpf *vsp1_rpf_create(struct vsp1_device > *vsp1, unsigned int index); struct vsp1_rwpf *vsp1_wpf_create(struct > vsp1_device *vsp1, unsigned int index); > > +void vsp1_wpf_stop(struct vsp1_rwpf *wpf); > + > int vsp1_rwpf_init_ctrls(struct vsp1_rwpf *rwpf, unsigned int ncontrols); > > -extern const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops; > +extern const struct v4l2_subdev_ops vsp1_rwpf_subdev_ops; > > struct v4l2_rect *vsp1_rwpf_get_crop(struct vsp1_rwpf *rwpf, > struct v4l2_subdev_state *sd_state); diff -- > git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c > b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c > index d0074ca00920..cab4445eca69 100644 > --- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c > +++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c > @@ -186,17 +186,13 @@ static int wpf_init_controls(struct vsp1_rwpf > *wpf) } > > /* ---------------------------------------------------------------------- > ------- > - * V4L2 Subdevice Core Operations > + * VSP1 Entity Operations > */ > > -static int wpf_s_stream(struct v4l2_subdev *subdev, int enable) > +void vsp1_wpf_stop(struct vsp1_rwpf *wpf) > { > - struct vsp1_rwpf *wpf = to_rwpf(subdev); > struct vsp1_device *vsp1 = wpf->entity.vsp1; > > - if (enable) > - return 0; > - > /* > * Write to registers directly when stopping the stream as there > will be > * no pipeline run to apply the display list. > @@ -204,27 +200,8 @@ static int wpf_s_stream(struct v4l2_subdev *subdev, > int enable) > vsp1_write(vsp1, VI6_WPF_IRQ_ENB(wpf->entity.index), 0); > vsp1_write(vsp1, wpf->entity.index * VI6_WPF_OFFSET + > VI6_WPF_SRCRPF, 0); > - > - return 0; > } > > -/* ---------------------------------------------------------------------- > ------- > - * V4L2 Subdevice Operations > - */ > - > -static const struct v4l2_subdev_video_ops wpf_video_ops = { > - .s_stream = wpf_s_stream, > -}; > - > -static const struct v4l2_subdev_ops wpf_ops = { > - .video = &wpf_video_ops, > - .pad = &vsp1_rwpf_pad_ops, > -}; > - > -/* ---------------------------------------------------------------------- > ------- > - * VSP1 Entity Operations > - */ > - > static void vsp1_wpf_destroy(struct vsp1_entity *entity) { > struct vsp1_rwpf *wpf = entity_to_rwpf(entity); @@ -583,7 +560,7 @@ > struct vsp1_rwpf *vsp1_wpf_create(struct vsp1_device *vsp1, unsigned int > index) > wpf->entity.index = index; > > sprintf(name, "wpf.%u", index); > - ret = vsp1_entity_init(vsp1, &wpf->entity, name, 2, &wpf_ops, > + ret = vsp1_entity_init(vsp1, &wpf->entity, name, 2, > +&vsp1_rwpf_subdev_ops, > MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER); > if (ret < 0) > return ERR_PTR(ret); > > base-commit: 19e67e01eb1e84f3529770d084b93f16a4894c42 > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c index f8093ba9539e..68d05243c3ee 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c +++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c @@ -373,7 +373,7 @@ int vsp1_pipeline_stop(struct vsp1_pipeline *pipe) (7 << VI6_DPR_SMPPT_TGW_SHIFT) | (VI6_DPR_NODE_UNUSED << VI6_DPR_SMPPT_PT_SHIFT)); - v4l2_subdev_call(&pipe->output->entity.subdev, video, s_stream, 0); + vsp1_wpf_stop(pipe->output); return ret; } diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c index 3b17f5fa4067..ea12c3f12c92 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_rpf.c +++ b/drivers/media/platform/renesas/vsp1/vsp1_rpf.c @@ -43,14 +43,6 @@ static inline void vsp1_rpf_write(struct vsp1_rwpf *rpf, data); } -/* ----------------------------------------------------------------------------- - * V4L2 Subdevice Operations - */ - -static const struct v4l2_subdev_ops rpf_ops = { - .pad = &vsp1_rwpf_pad_ops, -}; - /* ----------------------------------------------------------------------------- * VSP1 Entity Operations */ @@ -411,7 +403,7 @@ struct vsp1_rwpf *vsp1_rpf_create(struct vsp1_device *vsp1, unsigned int index) rpf->entity.index = index; sprintf(name, "rpf.%u", index); - ret = vsp1_entity_init(vsp1, &rpf->entity, name, 2, &rpf_ops, + ret = vsp1_entity_init(vsp1, &rpf->entity, name, 2, &vsp1_rwpf_subdev_ops, MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER); if (ret < 0) return ERR_PTR(ret); diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c index 22a82d218152..e0f87c8103ca 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.c @@ -24,7 +24,7 @@ struct v4l2_rect *vsp1_rwpf_get_crop(struct vsp1_rwpf *rwpf, } /* ----------------------------------------------------------------------------- - * V4L2 Subdevice Pad Operations + * V4L2 Subdevice Operations */ static int vsp1_rwpf_enum_mbus_code(struct v4l2_subdev *subdev, @@ -243,7 +243,7 @@ static int vsp1_rwpf_set_selection(struct v4l2_subdev *subdev, return ret; } -const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops = { +static const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops = { .init_cfg = vsp1_entity_init_cfg, .enum_mbus_code = vsp1_rwpf_enum_mbus_code, .enum_frame_size = vsp1_rwpf_enum_frame_size, @@ -253,6 +253,10 @@ const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops = { .set_selection = vsp1_rwpf_set_selection, }; +const struct v4l2_subdev_ops vsp1_rwpf_subdev_ops = { + .pad = &vsp1_rwpf_pad_ops, +}; + /* ----------------------------------------------------------------------------- * Controls */ diff --git a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.h b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.h index eac5c04c2239..e0d212c70b2f 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_rwpf.h +++ b/drivers/media/platform/renesas/vsp1/vsp1_rwpf.h @@ -79,9 +79,11 @@ static inline struct vsp1_rwpf *entity_to_rwpf(struct vsp1_entity *entity) struct vsp1_rwpf *vsp1_rpf_create(struct vsp1_device *vsp1, unsigned int index); struct vsp1_rwpf *vsp1_wpf_create(struct vsp1_device *vsp1, unsigned int index); +void vsp1_wpf_stop(struct vsp1_rwpf *wpf); + int vsp1_rwpf_init_ctrls(struct vsp1_rwpf *rwpf, unsigned int ncontrols); -extern const struct v4l2_subdev_pad_ops vsp1_rwpf_pad_ops; +extern const struct v4l2_subdev_ops vsp1_rwpf_subdev_ops; struct v4l2_rect *vsp1_rwpf_get_crop(struct vsp1_rwpf *rwpf, struct v4l2_subdev_state *sd_state); diff --git a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c index d0074ca00920..cab4445eca69 100644 --- a/drivers/media/platform/renesas/vsp1/vsp1_wpf.c +++ b/drivers/media/platform/renesas/vsp1/vsp1_wpf.c @@ -186,17 +186,13 @@ static int wpf_init_controls(struct vsp1_rwpf *wpf) } /* ----------------------------------------------------------------------------- - * V4L2 Subdevice Core Operations + * VSP1 Entity Operations */ -static int wpf_s_stream(struct v4l2_subdev *subdev, int enable) +void vsp1_wpf_stop(struct vsp1_rwpf *wpf) { - struct vsp1_rwpf *wpf = to_rwpf(subdev); struct vsp1_device *vsp1 = wpf->entity.vsp1; - if (enable) - return 0; - /* * Write to registers directly when stopping the stream as there will be * no pipeline run to apply the display list. @@ -204,27 +200,8 @@ static int wpf_s_stream(struct v4l2_subdev *subdev, int enable) vsp1_write(vsp1, VI6_WPF_IRQ_ENB(wpf->entity.index), 0); vsp1_write(vsp1, wpf->entity.index * VI6_WPF_OFFSET + VI6_WPF_SRCRPF, 0); - - return 0; } -/* ----------------------------------------------------------------------------- - * V4L2 Subdevice Operations - */ - -static const struct v4l2_subdev_video_ops wpf_video_ops = { - .s_stream = wpf_s_stream, -}; - -static const struct v4l2_subdev_ops wpf_ops = { - .video = &wpf_video_ops, - .pad = &vsp1_rwpf_pad_ops, -}; - -/* ----------------------------------------------------------------------------- - * VSP1 Entity Operations - */ - static void vsp1_wpf_destroy(struct vsp1_entity *entity) { struct vsp1_rwpf *wpf = entity_to_rwpf(entity); @@ -583,7 +560,7 @@ struct vsp1_rwpf *vsp1_wpf_create(struct vsp1_device *vsp1, unsigned int index) wpf->entity.index = index; sprintf(name, "wpf.%u", index); - ret = vsp1_entity_init(vsp1, &wpf->entity, name, 2, &wpf_ops, + ret = vsp1_entity_init(vsp1, &wpf->entity, name, 2, &vsp1_rwpf_subdev_ops, MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER); if (ret < 0) return ERR_PTR(ret);
The VSP1 driver uses the subdev .s_stream() operation to stop WPF instances, without a corresponding call to start them. The V4L2 subdev core started warning about unbalanced .s_stream() calls in commit 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() requirements"), causing a regression with this driver. Fix the problem by replacing the .s_stream() operation with an explicit function call for WPF instances. This allows sharing an additional data structure between RPF and WPF instances. Fixes: 009905ec5043 ("media: v4l2-subdev: Document and enforce .s_stream() requirements") Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Closes: https://lore.kernel.org/linux-media/2221395-6a9b-9527-d697-e76aebc6af@linux-m68k.org/ Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- .../media/platform/renesas/vsp1/vsp1_pipe.c | 2 +- .../media/platform/renesas/vsp1/vsp1_rpf.c | 10 +------ .../media/platform/renesas/vsp1/vsp1_rwpf.c | 8 +++-- .../media/platform/renesas/vsp1/vsp1_rwpf.h | 4 ++- .../media/platform/renesas/vsp1/vsp1_wpf.c | 29 ++----------------- 5 files changed, 14 insertions(+), 39 deletions(-) base-commit: 19e67e01eb1e84f3529770d084b93f16a4894c42