Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 010B9C6FD19 for ; Sun, 12 Mar 2023 14:05:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229985AbjCLOFn (ORCPT ); Sun, 12 Mar 2023 10:05:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53038 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229712AbjCLOFl (ORCPT ); Sun, 12 Mar 2023 10:05:41 -0400 Received: from comms.puri.sm (comms.puri.sm [159.203.221.185]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C5EE834F47; Sun, 12 Mar 2023 07:05:38 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by comms.puri.sm (Postfix) with ESMTP id A9477DFD63; Sun, 12 Mar 2023 07:05:38 -0700 (PDT) Received: from comms.puri.sm ([127.0.0.1]) by localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5zL5gddz04c6; Sun, 12 Mar 2023 07:05:37 -0700 (PDT) Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=puri.sm; s=comms; t=1678629936; bh=kSjmrOX7zwfR40OtFsmsGK97DSYPNpZa4eQh4aAaAaE=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=Sek7CHvtJK0ypZIVWZbJqesxRzDwUWyocURxFGg/nTJfBo1A0U2dsDnCYmCUVDj/P YPg0VZUfDb65mZM0uuUsTnTEYTkGiJP/ucPSEm1QnRRGaUXvfFJTVgEdPiu+yd7nu8 H5UeajwK7+ktwgYJPdqzIb5GSnLrb7QJHOQKQeiZsYsJn0keijx1UIgrcxbLsnFtR6 KDMqxjHgRBC9lvVHM4rI/1xYU7fWHz0Q1erY+R00W852yJlutU6uIl14S7F1+3/3wO hrGr6m2k1IfcN9NScqCOYjFlFaNOZtxf2h1SGduA/N/zVa3WcZSxh29UBHFLXpsaAD lkdGAXEu5fzXg== Subject: Re: [PATCH v2 1/2] media: imx: imx8mq-mipi-csi2: Use V4L2 subdev active state From: Martin Kepplinger To: Laurent Pinchart Cc: slongerbeam@gmail.com, p.zabel@pengutronix.de, mchehab@kernel.org, gregkh@linuxfoundation.org, shawnguo@kernel.org, kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel@puri.sm Date: Sun, 12 Mar 2023 15:05:32 +0100 In-Reply-To: <20230312133921.GK2545@pendragon.ideasonboard.com> References: <20230307150047.1486186-1-martin.kepplinger@puri.sm> <20230307150047.1486186-2-martin.kepplinger@puri.sm> <20230312133112.GH2545@pendragon.ideasonboard.com> <20230312133921.GK2545@pendragon.ideasonboard.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.3-1+deb11u1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am Sonntag, dem 12.03.2023 um 15:39 +0200 schrieb Laurent Pinchart: > On Sun, Mar 12, 2023 at 03:31:23PM +0200, Laurent Pinchart wrote: > > Hi Martin, > > > > Thank you for the patch. > > > > On Tue, Mar 07, 2023 at 04:00:46PM +0100, Martin Kepplinger wrote: > > > Simplify the driver by using the V4L2 subdev active state API to > > > store > > > the active format. > > > > > > Signed-off-by: Martin Kepplinger > > > --- > > >  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 118 ++++++++----- > > > ------ > > >  1 file changed, 46 insertions(+), 72 deletions(-) > > > > > > diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > > b/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > > index c0d0bf770096..1aa8622a3bae 100644 > > > --- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > > +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > > @@ -119,9 +119,7 @@ struct csi_state { > > >   > > >         struct v4l2_mbus_config_mipi_csi2 bus; > > >   > > > -       struct mutex lock; /* Protect csi2_fmt, format_mbus, > > > state, hs_settle */ > > > -       const struct csi2_pix_format *csi2_fmt; > > > -       struct v4l2_mbus_framefmt > > > format_mbus[MIPI_CSI2_PADS_NUM]; > > > +       struct mutex lock; /* Protect state and hs_settle */ > > >         u32 state; > > >         u32 hs_settle; > > >   > > > @@ -322,16 +320,23 @@ static int imx8mq_mipi_csi_clk_get(struct > > > csi_state *state) > > >         return devm_clk_bulk_get(state->dev, CSI2_NUM_CLKS, > > > state->clks); > > >  } > > >   > > > -static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state > > > *state) > > > +static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state > > > *state, > > > +                                         struct > > > v4l2_subdev_state *sd_state) > > >  { > > >         s64 link_freq; > > >         u32 lane_rate; > > >         unsigned long esc_clk_rate; > > >         u32 min_ths_settle, max_ths_settle, ths_settle_ns, > > > esc_clk_period_ns; > > > +       const struct v4l2_mbus_framefmt *fmt; > > > +       const struct csi2_pix_format *csi2_fmt; > > >   > > >         /* Calculate the line rate from the pixel rate. */ > > > + > > > +       fmt = v4l2_subdev_get_pad_format(&state->sd, sd_state, > > > MIPI_CSI2_PAD_SINK); > > > +       csi2_fmt = find_csi2_format(fmt->code); > > > + > > >         link_freq = v4l2_get_link_freq(state->src_sd- > > > >ctrl_handler, > > > -                                      state->csi2_fmt->width, > > > +                                      csi2_fmt->width, > > >                                        state->bus.num_data_lanes > > > * 2); > > >         if (link_freq < 0) { > > >                 dev_err(state->dev, "Unable to obtain link > > > frequency: %d\n", > > > @@ -380,7 +385,8 @@ static int > > > imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state) > > >         return 0; > > >  } > > >   > > > -static int imx8mq_mipi_csi_start_stream(struct csi_state *state) > > > +static int imx8mq_mipi_csi_start_stream(struct csi_state *state, > > > +                                       struct v4l2_subdev_state > > > *sd_state) > > >  { > > >         int ret; > > >   > > > @@ -389,7 +395,7 @@ static int > > > imx8mq_mipi_csi_start_stream(struct csi_state *state) > > >                 return ret; > > >   > > >         imx8mq_mipi_csi_set_params(state); > > > -       ret = imx8mq_mipi_csi_calc_hs_settle(state); > > > +       ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state); > > >         if (ret) > > >                 return ret; > > >   > > > @@ -415,6 +421,7 @@ static struct csi_state > > > *mipi_sd_to_csi2_state(struct v4l2_subdev *sdev) > > >  static int imx8mq_mipi_csi_s_stream(struct v4l2_subdev *sd, int > > > enable) > > >  { > > >         struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > +       struct v4l2_subdev_state *sd_state; > > >         int ret = 0; > > >   > > >         if (enable) { > > > @@ -431,7 +438,9 @@ static int imx8mq_mipi_csi_s_stream(struct > > > v4l2_subdev *sd, int enable) > > >                         goto unlock; > > >                 } > > >   > > > -               ret = imx8mq_mipi_csi_start_stream(state); > > > +               sd_state = > > > v4l2_subdev_lock_and_get_active_state(sd); > > > + > > > +               ret = imx8mq_mipi_csi_start_stream(state, > > > sd_state); > > >                 if (ret < 0) > > >                         goto unlock; > > > > You're leaving the state locked here. I would write > > > >                 sd_state = > > v4l2_subdev_lock_and_get_active_state(sd); > >                 ret = imx8mq_mipi_csi_start_stream(state, > > sd_state); > >                 v4l2_subdev_unlock_state(sd_state); > > > >                 if (ret < 0) > >                         goto unlock; > > > > and drop the v4l2_subdev_unlock_state() call below. Apart from > > that, > > > > Reviewed-by: Laurent Pinchart > > If you're fine with this change, there's no need so submit a v3, I'll > handle this locally. Please do. I'm fine with this change, yes. Thanks a lot, > > > >   > > > @@ -440,6 +449,8 @@ static int imx8mq_mipi_csi_s_stream(struct > > > v4l2_subdev *sd, int enable) > > >                         goto unlock; > > >   > > >                 state->state |= ST_STREAMING; > > > + > > > +               v4l2_subdev_unlock_state(sd_state); > > >         } else { > > >                 v4l2_subdev_call(state->src_sd, video, s_stream, > > > 0); > > >                 imx8mq_mipi_csi_stop_stream(state); > > > @@ -455,29 +466,14 @@ static int imx8mq_mipi_csi_s_stream(struct > > > v4l2_subdev *sd, int enable) > > >         return ret; > > >  } > > >   > > > -static struct v4l2_mbus_framefmt * > > > -imx8mq_mipi_csi_get_format(struct csi_state *state, > > > -                          struct v4l2_subdev_state *sd_state, > > > -                          enum v4l2_subdev_format_whence which, > > > -                          unsigned int pad) > > > -{ > > > -       if (which == V4L2_SUBDEV_FORMAT_TRY) > > > -               return v4l2_subdev_get_try_format(&state->sd, > > > sd_state, pad); > > > - > > > -       return &state->format_mbus[pad]; > > > -} > > > - > > >  static int imx8mq_mipi_csi_init_cfg(struct v4l2_subdev *sd, > > >                                     struct v4l2_subdev_state > > > *sd_state) > > >  { > > > -       struct csi_state *state = mipi_sd_to_csi2_state(sd); > > >         struct v4l2_mbus_framefmt *fmt_sink; > > >         struct v4l2_mbus_framefmt *fmt_source; > > > -       enum v4l2_subdev_format_whence which; > > >   > > > -       which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : > > > V4L2_SUBDEV_FORMAT_ACTIVE; > > > -       fmt_sink = imx8mq_mipi_csi_get_format(state, sd_state, > > > which, > > > -                                             > > > MIPI_CSI2_PAD_SINK); > > > +       fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state, > > > MIPI_CSI2_PAD_SINK); > > > +       fmt_source = v4l2_subdev_get_pad_format(sd, sd_state, > > > MIPI_CSI2_PAD_SOURCE); > > >   > > >         fmt_sink->code = MEDIA_BUS_FMT_SGBRG10_1X10; > > >         fmt_sink->width = MIPI_CSI2_DEF_PIX_WIDTH; > > > @@ -491,38 +487,15 @@ static int imx8mq_mipi_csi_init_cfg(struct > > > v4l2_subdev *sd, > > >                 V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink- > > > >colorspace, > > >                                               fmt_sink- > > > >ycbcr_enc); > > >   > > > -       fmt_source = imx8mq_mipi_csi_get_format(state, sd_state, > > > which, > > > - > > >                                                MIPI_CSI2_PAD_SOURC > > > E); > > >         *fmt_source = *fmt_sink; > > >   > > >         return 0; > > >  } > > >   > > > -static int imx8mq_mipi_csi_get_fmt(struct v4l2_subdev *sd, > > > -                                  struct v4l2_subdev_state > > > *sd_state, > > > -                                  struct v4l2_subdev_format > > > *sdformat) > > > -{ > > > -       struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > -       struct v4l2_mbus_framefmt *fmt; > > > - > > > -       fmt = imx8mq_mipi_csi_get_format(state, sd_state, > > > sdformat->which, > > > -                                        sdformat->pad); > > > - > > > -       mutex_lock(&state->lock); > > > - > > > -       sdformat->format = *fmt; > > > - > > > -       mutex_unlock(&state->lock); > > > - > > > -       return 0; > > > -} > > > - > > >  static int imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev > > > *sd, > > >                                           struct > > > v4l2_subdev_state *sd_state, > > >                                           struct > > > v4l2_subdev_mbus_code_enum *code) > > >  { > > > -       struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > - > > >         /* > > >          * We can't transcode in any way, the source format is > > > identical > > >          * to the sink format. > > > @@ -533,8 +506,7 @@ static int > > > imx8mq_mipi_csi_enum_mbus_code(struct v4l2_subdev *sd, > > >                 if (code->index > 0) > > >                         return -EINVAL; > > >   > > > -               fmt = imx8mq_mipi_csi_get_format(state, sd_state, > > > code->which, > > > -                                                code->pad); > > > +               fmt = v4l2_subdev_get_pad_format(sd, sd_state, > > > code->pad); > > >                 code->code = fmt->code; > > >                 return 0; > > >         } > > > @@ -554,8 +526,7 @@ static int imx8mq_mipi_csi_set_fmt(struct > > > v4l2_subdev *sd, > > >                                    struct v4l2_subdev_state > > > *sd_state, > > >                                    struct v4l2_subdev_format > > > *sdformat) > > >  { > > > -       struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > -       struct csi2_pix_format const *csi2_fmt; > > > +       const struct csi2_pix_format *csi2_fmt; > > >         struct v4l2_mbus_framefmt *fmt; > > >   > > >         /* > > > @@ -563,7 +534,7 @@ static int imx8mq_mipi_csi_set_fmt(struct > > > v4l2_subdev *sd, > > >          * modified. > > >          */ > > >         if (sdformat->pad == MIPI_CSI2_PAD_SOURCE) > > > -               return imx8mq_mipi_csi_get_fmt(sd, sd_state, > > > sdformat); > > > +               return v4l2_subdev_get_fmt(sd, sd_state, > > > sdformat); > > >   > > >         if (sdformat->pad != MIPI_CSI2_PAD_SINK) > > >                 return -EINVAL; > > > @@ -572,10 +543,7 @@ static int imx8mq_mipi_csi_set_fmt(struct > > > v4l2_subdev *sd, > > >         if (!csi2_fmt) > > >                 csi2_fmt = &imx8mq_mipi_csi_formats[0]; > > >   > > > -       fmt = imx8mq_mipi_csi_get_format(state, sd_state, > > > sdformat->which, > > > -                                        sdformat->pad); > > > - > > > -       mutex_lock(&state->lock); > > > +       fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat- > > > >pad); > > >   > > >         fmt->code = csi2_fmt->code; > > >         fmt->width = sdformat->format.width; > > > @@ -584,16 +552,9 @@ static int imx8mq_mipi_csi_set_fmt(struct > > > v4l2_subdev *sd, > > >         sdformat->format = *fmt; > > >   > > >         /* Propagate the format from sink to source. */ > > > -       fmt = imx8mq_mipi_csi_get_format(state, sd_state, > > > sdformat->which, > > > -                                        MIPI_CSI2_PAD_SOURCE); > > > +       fmt = v4l2_subdev_get_pad_format(sd, sd_state, > > > MIPI_CSI2_PAD_SOURCE); > > >         *fmt = sdformat->format; > > >   > > > -       /* Store the CSI2 format descriptor for active formats. > > > */ > > > -       if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > > -               state->csi2_fmt = csi2_fmt; > > > - > > > -       mutex_unlock(&state->lock); > > > - > > >         return 0; > > >  } > > >   > > > @@ -604,7 +565,7 @@ static const struct v4l2_subdev_video_ops > > > imx8mq_mipi_csi_video_ops = { > > >  static const struct v4l2_subdev_pad_ops imx8mq_mipi_csi_pad_ops > > > = { > > >         .init_cfg               = imx8mq_mipi_csi_init_cfg, > > >         .enum_mbus_code         = imx8mq_mipi_csi_enum_mbus_code, > > > -       .get_fmt                = imx8mq_mipi_csi_get_fmt, > > > +       .get_fmt                = v4l2_subdev_get_fmt, > > >         .set_fmt                = imx8mq_mipi_csi_set_fmt, > > >  }; > > >   > > > @@ -732,6 +693,7 @@ static int imx8mq_mipi_csi_pm_resume(struct > > > device *dev) > > >  { > > >         struct v4l2_subdev *sd = dev_get_drvdata(dev); > > >         struct csi_state *state = mipi_sd_to_csi2_state(sd); > > > +       struct v4l2_subdev_state *sd_state; > > >         int ret = 0; > > >   > > >         mutex_lock(&state->lock); > > > @@ -741,7 +703,9 @@ static int imx8mq_mipi_csi_pm_resume(struct > > > device *dev) > > >                 ret = imx8mq_mipi_csi_clk_enable(state); > > >         } > > >         if (state->state & ST_STREAMING) { > > > -               ret = imx8mq_mipi_csi_start_stream(state); > > > +               sd_state = > > > v4l2_subdev_lock_and_get_active_state(sd); > > > +               ret = imx8mq_mipi_csi_start_stream(state, > > > sd_state); > > > +               v4l2_subdev_unlock_state(sd_state); > > >                 if (ret) > > >                         goto unlock; > > >         } > > > @@ -821,6 +785,7 @@ static const struct dev_pm_ops > > > imx8mq_mipi_csi_pm_ops = { > > >  static int imx8mq_mipi_csi_subdev_init(struct csi_state *state) > > >  { > > >         struct v4l2_subdev *sd = &state->sd; > > > +       int ret; > > >   > > >         v4l2_subdev_init(sd, &imx8mq_mipi_csi_subdev_ops); > > >         sd->owner = THIS_MODULE; > > > @@ -834,15 +799,22 @@ static int > > > imx8mq_mipi_csi_subdev_init(struct csi_state *state) > > >   > > >         sd->dev = state->dev; > > >   > > > -       state->csi2_fmt = &imx8mq_mipi_csi_formats[0]; > > > -       imx8mq_mipi_csi_init_cfg(sd, NULL); > > > - > > >         state->pads[MIPI_CSI2_PAD_SINK].flags = MEDIA_PAD_FL_SINK > > >                                          | > > > MEDIA_PAD_FL_MUST_CONNECT; > > >         state->pads[MIPI_CSI2_PAD_SOURCE].flags = > > > MEDIA_PAD_FL_SOURCE > > >                                            | > > > MEDIA_PAD_FL_MUST_CONNECT; > > > -       return media_entity_pads_init(&sd->entity, > > > MIPI_CSI2_PADS_NUM, > > > -                                     state->pads); > > > +       ret = media_entity_pads_init(&sd->entity, > > > MIPI_CSI2_PADS_NUM, > > > +                                    state->pads); > > > +       if (ret) > > > +               return ret; > > > + > > > +       ret = v4l2_subdev_init_finalize(sd); > > > +       if (ret) { > > > +               media_entity_cleanup(&sd->entity); > > > +               return ret; > > > +       } > > > + > > > +       return 0; > > >  } > > >   > > >  static void imx8mq_mipi_csi_release_icc(struct platform_device > > > *pdev) > > > @@ -968,6 +940,7 @@ static int imx8mq_mipi_csi_probe(struct > > > platform_device *pdev) > > >         imx8mq_mipi_csi_runtime_suspend(&pdev->dev); > > >   > > >         media_entity_cleanup(&state->sd.entity); > > > +       v4l2_subdev_cleanup(&state->sd); > > >         v4l2_async_nf_unregister(&state->notifier); > > >         v4l2_async_nf_cleanup(&state->notifier); > > >         v4l2_async_unregister_subdev(&state->sd); > > > @@ -991,6 +964,7 @@ static int imx8mq_mipi_csi_remove(struct > > > platform_device *pdev) > > >         pm_runtime_disable(&pdev->dev); > > >         imx8mq_mipi_csi_runtime_suspend(&pdev->dev); > > >         media_entity_cleanup(&state->sd.entity); > > > +       v4l2_subdev_cleanup(&state->sd); > > >         mutex_destroy(&state->lock); > > >         pm_runtime_set_suspended(&pdev->dev); > > >         imx8mq_mipi_csi_release_icc(pdev); >