| Message ID | 1432855083-25969-1-git-send-email-sakari.ailus@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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); >
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);
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.
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);
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(-)