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

Message ID 1432855083-25969-1-git-send-email-sakari.ailus@iki.fi
State New
Headers show

Commit Message

Sakari Ailus May 28, 2015, 11:17 p.m. UTC
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(-)

Comments

Laurent Pinchart June 10, 2015, 12:52 a.m. UTC | #1
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);
Sakari Ailus June 10, 2015, 9:38 p.m. UTC | #2
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);
>
Laurent Pinchart June 12, 2015, 7:24 a.m. UTC | #3
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);
Sakari Ailus June 12, 2015, 11:01 p.m. UTC | #4
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.

Patch
diff mbox

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);