[PATCH 1/1] omap3isp: Fix sub-device power management code

[PATCH 1/1] omap3isp: Fix sub-device power management code

[Sakari Ailus]

The power management code was reworked a little due to interface changes in
the MC. Due to those changes the power management broke a bit, fix it so the
functionality is reverted to old behaviour.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Fixes: 813f5c0ac5cc [media] media: Change media device link_notify behaviour
Cc: stable@vger.kernel.org # since v3.10
---
 drivers/media/platform/omap3isp/isp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index a038c05..3e6b97b 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -829,14 +829,14 @@ static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
 	int ret;
 
 	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
-	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
+	    !(flags & MEDIA_LNK_FL_ENABLED)) {
 		/* Powering off entities is assumed to never fail. */
 		isp_pipeline_pm_power(source, -sink_use);
 		isp_pipeline_pm_power(sink, -source_use);
 		return 0;
 	}
 
-	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
 		(flags & MEDIA_LNK_FL_ENABLED)) {
 
 		ret = isp_pipeline_pm_power(source, sink_use);
-- 
2.1.4

Re: [PATCH 1/1] omap3isp: Fix sub-device power management code

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday 29 May 2015 02:17:47 Sakari Ailus wrote:
> The power management code was reworked a little due to interface changes in
> the MC. Due to those changes the power management broke a bit, fix it so the
> functionality is reverted to old behaviour.

I found the commit message a bit vague. How about

"Commit 813f5c0ac5cc ("media: Change media device link_notify behaviour") 
modified the media controller link setup notification API and updated the 
OMAP3 ISP driver accordingly. As a side effect it introduced a bug by turning 
power on after setting the link instead of before. This results in powered off 
entities being accessed. Fix it."

Or have I misunderstood the problem ?

> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Fixes: 813f5c0ac5cc [media] media: Change media device link_notify behaviour
> Cc: stable@vger.kernel.org # since v3.10
> ---
>  drivers/media/platform/omap3isp/isp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index a038c05..3e6b97b 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -829,14 +829,14 @@ static int isp_pipeline_link_notify(struct media_link
> *link, u32 flags, int ret;
> 
>  	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> -	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
> +	    !(flags & MEDIA_LNK_FL_ENABLED)) {

Isn't link->flags == flags in the post notification callback ?

>  		/* Powering off entities is assumed to never fail. */
>  		isp_pipeline_pm_power(source, -sink_use);
>  		isp_pipeline_pm_power(sink, -source_use);
>  		return 0;
>  	}
> 
> -	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
>  		(flags & MEDIA_LNK_FL_ENABLED)) {
> 
>  		ret = isp_pipeline_pm_power(source, sink_use);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] omap3isp: Fix sub-device power management code

[Sakari Ailus]

Hi Laurent,

On Wed, Jun 10, 2015 at 03:52:50AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday 29 May 2015 02:17:47 Sakari Ailus wrote:
> > The power management code was reworked a little due to interface changes in
> > the MC. Due to those changes the power management broke a bit, fix it so the
> > functionality is reverted to old behaviour.
> 
> I found the commit message a bit vague. How about
> 
> "Commit 813f5c0ac5cc ("media: Change media device link_notify behaviour") 
> modified the media controller link setup notification API and updated the 
> OMAP3 ISP driver accordingly. As a side effect it introduced a bug by turning 
> power on after setting the link instead of before. This results in powered off 
> entities being accessed. Fix it."
> 
> Or have I misunderstood the problem ?

Not entirely, but it's not just that: depending on the order in which the
links are changed and the video nodes opened or closed, the use counts may
end up being too high or too low (even negative).

> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > Fixes: 813f5c0ac5cc [media] media: Change media device link_notify behaviour
> > Cc: stable@vger.kernel.org # since v3.10
> > ---
> >  drivers/media/platform/omap3isp/isp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index a038c05..3e6b97b 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -829,14 +829,14 @@ static int isp_pipeline_link_notify(struct media_link
> > *link, u32 flags, int ret;
> > 
> >  	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> > -	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
> > +	    !(flags & MEDIA_LNK_FL_ENABLED)) {
> 
> Isn't link->flags == flags in the post notification callback ?

It is. IMO it's better to use flags here still. So below is the actual
functional change.

> 
> >  		/* Powering off entities is assumed to never fail. */
> >  		isp_pipeline_pm_power(source, -sink_use);
> >  		isp_pipeline_pm_power(sink, -source_use);
> >  		return 0;
> >  	}
> > 
> > -	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> > +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> >  		(flags & MEDIA_LNK_FL_ENABLED)) {
> > 
> >  		ret = isp_pipeline_pm_power(source, sink_use);
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/1] omap3isp: Fix sub-device power management code

[Laurent Pinchart]

Hi Sakari,

On Thursday 11 June 2015 00:38:11 Sakari Ailus wrote:
> On Wed, Jun 10, 2015 at 03:52:50AM +0300, Laurent Pinchart wrote:
> > On Friday 29 May 2015 02:17:47 Sakari Ailus wrote:
> > > The power management code was reworked a little due to interface changes
> > > in the MC. Due to those changes the power management broke a bit, fix it
> > > so the functionality is reverted to old behaviour.
> > 
> > I found the commit message a bit vague. How about
> > 
> > "Commit 813f5c0ac5cc ("media: Change media device link_notify behaviour")
> > modified the media controller link setup notification API and updated the
> > OMAP3 ISP driver accordingly. As a side effect it introduced a bug by
> > turning power on after setting the link instead of before. This results
> > in powered off entities being accessed. Fix it."
> > 
> > Or have I misunderstood the problem ?
> 
> Not entirely, but it's not just that: depending on the order in which the
> links are changed and the video nodes opened or closed, the use counts may
> end up being too high or too low (even negative).

OK. Could you please update the commit message accordingly ?

> >> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >> Fixes: 813f5c0ac5cc [media] media: Change media device link_notify
> >> behaviour Cc: stable@vger.kernel.org # since v3.10
> >> ---
> >> 
> >>  drivers/media/platform/omap3isp/isp.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index a038c05..3e6b97b 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -829,14 +829,14 @@ static int isp_pipeline_link_notify(struct
> >> media_link *link, u32 flags,
> >> 	int ret;
> >> 
> >>  	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> >> -	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
> >> +	    !(flags & MEDIA_LNK_FL_ENABLED)) {
> > 
> > Isn't link->flags == flags in the post notification callback ?
> 
> It is. IMO it's better to use flags here still. So below is the actual
> functional change.

No issue with that.

> >>  		/* Powering off entities is assumed to never fail. */
> >>  		isp_pipeline_pm_power(source, -sink_use);
> >>  		isp_pipeline_pm_power(sink, -source_use);
> >>  		return 0;
> >>  	}
> >> 
> >> -	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> >> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> >>  		(flags & MEDIA_LNK_FL_ENABLED)) {
> >>  		ret = isp_pipeline_pm_power(source, sink_use);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] omap3isp: Fix sub-device power management code

[Sakari Ailus]

Hi Laurent,

On Fri, Jun 12, 2015 at 10:24:28AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thursday 11 June 2015 00:38:11 Sakari Ailus wrote:
> > On Wed, Jun 10, 2015 at 03:52:50AM +0300, Laurent Pinchart wrote:
> > > On Friday 29 May 2015 02:17:47 Sakari Ailus wrote:
> > > > The power management code was reworked a little due to interface changes
> > > > in the MC. Due to those changes the power management broke a bit, fix it
> > > > so the functionality is reverted to old behaviour.
> > > 
> > > I found the commit message a bit vague. How about
> > > 
> > > "Commit 813f5c0ac5cc ("media: Change media device link_notify behaviour")
> > > modified the media controller link setup notification API and updated the
> > > OMAP3 ISP driver accordingly. As a side effect it introduced a bug by
> > > turning power on after setting the link instead of before. This results
> > > in powered off entities being accessed. Fix it."
> > > 
> > > Or have I misunderstood the problem ?
> > 
> > Not entirely, but it's not just that: depending on the order in which the
> > links are changed and the video nodes opened or closed, the use counts may
> > end up being too high or too low (even negative).
> 
> OK. Could you please update the commit message accordingly ?

Hmm. I'm still not fully certain how did I manage to reproduce that, but in
a few occasions the sensor was powered down when it shouldn't have been and
the power count was decreased more than it was first increased.

What's indeed easy to see is that in post notification of enabling the links
the use counts of the partial pipelines are in fact not those of the
partial, but the complete one, and thus end up being twice as much they
should be.

I'm fine with using the commit message you suggested, the bottom line still
is that it was broken, and the patch fixes it.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v1.1 1/1] omap3isp: Fix sub-device power management code

[Sakari Ailus]

Commit 813f5c0ac5cc ("media: Change media device link_notify behaviour")
modified the media controller link setup notification API and updated the
OMAP3 ISP driver accordingly. As a side effect it introduced a bug by
turning power on after setting the link instead of before. This results in
sub-devices not being powered down in some cases when they should be. Fix
it.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Fixes: 813f5c0ac5cc [media] media: Change media device link_notify behaviour
Cc: stable@vger.kernel.org # since v3.10
---
Hi Laurent,

I amended the commit message a bit. Let me know if you're ok with it.

 drivers/media/platform/omap3isp/isp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 6bcab28..ce0556c 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -829,14 +829,14 @@ static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
 	int ret;
 
 	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
-	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
+	    !(flags & MEDIA_LNK_FL_ENABLED)) {
 		/* Powering off entities is assumed to never fail. */
 		isp_pipeline_pm_power(source, -sink_use);
 		isp_pipeline_pm_power(sink, -source_use);
 		return 0;
 	}
 
-	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
 		(flags & MEDIA_LNK_FL_ENABLED)) {
 
 		ret = isp_pipeline_pm_power(source, sink_use);
-- 
2.1.4

Re: [PATCH v1.1 1/1] omap3isp: Fix sub-device power management code

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Saturday 13 June 2015 02:06:23 Sakari Ailus wrote:
> Commit 813f5c0ac5cc ("media: Change media device link_notify behaviour")
> modified the media controller link setup notification API and updated the
> OMAP3 ISP driver accordingly. As a side effect it introduced a bug by
> turning power on after setting the link instead of before. This results in
> sub-devices not being powered down in some cases when they should be. Fix
> it.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Fixes: 813f5c0ac5cc [media] media: Change media device link_notify behaviour
> Cc: stable@vger.kernel.org # since v3.10

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Applied to my tree, and pull request sent.

> ---
> Hi Laurent,
> 
> I amended the commit message a bit. Let me know if you're ok with it.
> 
>  drivers/media/platform/omap3isp/isp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 6bcab28..ce0556c 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -829,14 +829,14 @@ static int isp_pipeline_link_notify(struct media_link
> *link, u32 flags, int ret;
> 
>  	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> -	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
> +	    !(flags & MEDIA_LNK_FL_ENABLED)) {
>  		/* Powering off entities is assumed to never fail. */
>  		isp_pipeline_pm_power(source, -sink_use);
>  		isp_pipeline_pm_power(sink, -source_use);
>  		return 0;
>  	}
> 
> -	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
>  		(flags & MEDIA_LNK_FL_ENABLED)) {
> 
>  		ret = isp_pipeline_pm_power(source, sink_use);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1.1 1/1] omap3isp: Fix sub-device power management code

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Saturday 13 June 2015 02:06:23 Sakari Ailus wrote:
>> Commit 813f5c0ac5cc ("media: Change media device link_notify behaviour")
>> modified the media controller link setup notification API and updated the
>> OMAP3 ISP driver accordingly. As a side effect it introduced a bug by
>> turning power on after setting the link instead of before. This results in
>> sub-devices not being powered down in some cases when they should be. Fix
>> it.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>> Fixes: 813f5c0ac5cc [media] media: Change media device link_notify behaviour
>> Cc: stable@vger.kernel.org # since v3.10
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Applied to my tree, and pull request sent.

Thanks!

-- 
Sakari Ailus
sakari.ailus@iki.fi