Re: [PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday, 20 September 2017 12:16:54 EET Kieran Bingham wrote:
> When used as part of a display pipeline, the VSP is stopped and
> restarted explicitly by the DU from its suspend and resume handlers.
> There is thus no need to stop or restart pipelines in the VSP suspend
> and resume handlers, and doing so would cause the hardware to be
> left in a misconfigured state.
> 
> Ensure that the VSP suspend and resume handlers do not affect DRM
> based pipelines.

s/DRM-base/DRM-based/

> Signed-off-by: Kieran Bingham 
> ---
>  drivers/media/platform/vsp1/vsp1_drv.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c index 962e4c304076..ed25ba9d551b
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct device
> *dev) {
>   struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> 
> - vsp1_pipelines_suspend(vsp1);
> + /*
> +  * When used as part of a display pipeline, the VSP is stopped and
> +  * restarted explicitly by the DU

s/DU/DU./

> +  */
> + if (!vsp1->drm)
> + vsp1_pipelines_suspend(vsp1);
> +
>   pm_runtime_force_suspend(vsp1->dev);
> 
>   return 0;
> @@ -582,7 +588,13 @@ static int __maybe_unused vsp1_pm_resume(struct device
> *dev) struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> 
>   pm_runtime_force_resume(vsp1->dev);
> - vsp1_pipelines_resume(vsp1);
> +
> + /*
> +  * When used as part of a display pipeline, the VSP is stopped and
> +  * restarted explicitly by the DU

s/DU/DU./

Apart from that,

Reviewed-by: Laurent Pinchart 

> +  */
> + if (!vsp1->drm)
> + vsp1_pipelines_resume(vsp1);
> 
>   return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v10 2/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Sakari Ailus]

Hi Laurent,

On Sat, Nov 11, 2017 at 08:17:59AM +0200, Laurent Pinchart wrote:
> > > +static int rcar_csi2_s_power(struct v4l2_subdev *sd, int on)
> > > +{
> > > + struct rcar_csi2 *priv = sd_to_csi2(sd);
> > > +
> > > + if (on)
> > > + pm_runtime_get_sync(priv->dev);
> > > + else
> > > + pm_runtime_put(priv->dev);
> > 
> > The pipeline you have is already rather complex. Would it be an option to
> > power the hardware on when streaming is started? The smiapp driver does
> > this, without even implementing the s_power callback. (You'd still need to
> > call it on the image source, as long as we have drivers that need it.)
> 
> We've briefly discussed this before, and I agree that pipeline power 
> management needs to be redesigned, but we still haven't agreed on a proper 
> architecture for that. Feel free to propose an RFC :-) In the meantime I 

Well, perhaps we should look at the use cases and see if there are gaps.
Runtime PM is essentially used everywhere else in the kernel.

> wouldn't try to enforce one specific model.

What I just wanted to point out that by switching to runtime PM you avoid
adding a new driver which is dependent on the s_power callback which I
don't think we'll want to keep in the long run. In this case there's no
benefit from s_power in any quantifiable way that I can see.

Actually the master driver manages calling the s_power callback so there's
hardly a need to do so here. It's just that the master drivers still need
that as long as there are sub-device drivers that depend on it.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v10 2/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Sakari Ailus]

Hejssan, Niklas!

On Sat, Nov 11, 2017 at 01:11:13AM +0100, Niklas Söderlund wrote:
> Hej Sakari,
> 
> On 2017-11-11 00:32:27 +0200, Sakari Ailus wrote:
> > Hej Niklas,
> > 
> > Tack för uppdaterade lappar! Jag har några kommentar nedan... det ser bra
> > ut överallt.
> 
> Tack för din feedback!

Var så god!

...

> > > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv,
> > > +  struct v4l2_subdev *source,
> > > +  struct v4l2_mbus_framefmt *mf,
> > > +  u32 *phypll)
> > > +{
> > > + const struct phypll_hsfreqrange *hsfreq;
> > > + const struct rcar_csi2_format *format;
> > > + struct v4l2_ctrl *ctrl;
> > > + u64 mbps;
> > > +
> > > + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > 
> > How about LINK_FREQ instead? It'd be more straightforward to calculate
> > this. Up to you.
> 
> I need to use PIXEL_RATE as my test setup uses the adv748x driver which 
> only implement that control. In the short term I would like to support 
> both, but I need a setup to test that before I can add support for it.  
> In the long term, maybe we should deprecate one of the controls?

Hmm. At least we musn't give the responsibility to calculate the pixel rate
to the end user: this depends on the bus and that is not visible to the
user space.

The link frequency is usually chosen from a few alternatives (or there's
just a single choice). And the pixel rate depends on the pixel code chosen
--- so you'd have a menu with menu entries changing based on format. That'd
be odd.

Perhaps just leave it as-is, as suggested by Laurent?

...

> > > +
> > > + dev_dbg(priv->dev, "Using source %s pad: %u\n", source->name,
> > > + source_pad->index);
> > > +
> > > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > + fmt.pad = source_pad->index;
> > > + ret = v4l2_subdev_call(source, pad, get_fmt, NULL, );
> > > + if (ret)
> > > + return ret;
> > 
> > The link format has been validated already by this point this isn't
> > supposed to fail or produce different results than in link validation.
> > 
> > You could get the same format from the local pad, too.
> 
> Another good catch, I will use the format stored locally to make the 
> code simpler. As you state the formats are already validated so it's 
> safe to do so. I still need to get the remote subdevice to be able to 
> read the PIXEL_RATE control, but I can move that to 
> rcar_csi2_calc_phypll() where it's actually used.

Sounds good to me.
 
...

> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + struct v4l2_subdev_format *format)
> > > +{
> > > + struct rcar_csi2 *priv = sd_to_csi2(sd);
> > > + struct v4l2_mbus_framefmt *framefmt;
> > > +
> > 
> > Is everything possible? For instance, are there limits regarding width or
> > height? Or the pixel code? They should be checked here.
> 
> Yes it do care about pixel code which was only validated at s_stream() 
> time, will move the check here thanks!
> 
> > 
> > Also the format on the source pad is, presumably, dependent on the format
> > on the sink pad.
> 
> Yes, but since this driver can't change the format in any way is there 
> any harm in mirror whatever is set on one pad on the other(s)? This will 
> change once multiple stream support is added in a later series.

If the hardware does not have functionality that may affect the size or the
pixel code, then the format on the source pad shouldn't be settable.

Yes, this will change when support for multiple streams is added. You'll
get more pads, and the sink pad no longer will have a format. But... it's a
good question how to prepare for this --- is it possible without changing
the user space API? I'm sure we'll have other such cases, such as the
smiapp driver with sensor embedded data.

...

> > > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> > > +{
> > > + struct device_node *ep;
> > > + struct v4l2_fwnode_endpoint v4l2_ep;
> > > + int ret;
> > > +
> > > + ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > > + if (!ep) {
> > > + dev_err(priv->dev, "Not connected to subdevice\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), _ep);
> > > + if (ret) {
> > > + dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> > > + of_node_put(ep);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = rcar_csi2_parse_v4l2(priv, _ep);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + priv->remote.match.fwnode.fwnode =
> > > + fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > > + priv->remote.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > +
> > > + of_node_put(ep);
> > > +
> > > + priv->notifier.subdevs = devm_kzalloc(priv->dev,
> > > +   

Re: [PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines

[Kieran Bingham]

Ping ...

This patch appears to have got lost in the post.

Could someone pick it up please?

--
Regards

Kieran

On 20/09/17 10:16, Kieran Bingham wrote:
> When used as part of a display pipeline, the VSP is stopped and
> restarted explicitly by the DU from its suspend and resume handlers.
> There is thus no need to stop or restart pipelines in the VSP suspend
> and resume handlers, and doing so would cause the hardware to be
> left in a misconfigured state.
> 
> Ensure that the VSP suspend and resume handlers do not affect DRM
> based pipelines.
> 
> Signed-off-by: Kieran Bingham 
> ---
>  drivers/media/platform/vsp1/vsp1_drv.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index 962e4c304076..ed25ba9d551b 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct device 
> *dev)
>  {
>   struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>  
> - vsp1_pipelines_suspend(vsp1);
> + /*
> +  * When used as part of a display pipeline, the VSP is stopped and
> +  * restarted explicitly by the DU
> +  */
> + if (!vsp1->drm)
> + vsp1_pipelines_suspend(vsp1);
> +
>   pm_runtime_force_suspend(vsp1->dev);
>  
>   return 0;
> @@ -582,7 +588,13 @@ static int __maybe_unused vsp1_pm_resume(struct device 
> *dev)
>   struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>  
>   pm_runtime_force_resume(vsp1->dev);
> - vsp1_pipelines_resume(vsp1);
> +
> + /*
> +  * When used as part of a display pipeline, the VSP is stopped and
> +  * restarted explicitly by the DU
> +  */
> + if (!vsp1->drm)
> + vsp1_pipelines_resume(vsp1);
>  
>   return 0;
>  }
>