| Message ID | 1448992031-8271-15-git-send-email-subhransu.s.prusty@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 01 Dec 2015 18:47:11 +0100, Subhransu S. Prusty wrote: > > From: Ramesh Babu <ramesh.babu@intel.com> > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > --- > sound/soc/codecs/hdac_hdmi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c > index 46aa5cc..fa3cfa5 100644 > --- a/sound/soc/codecs/hdac_hdmi.c > +++ b/sound/soc/codecs/hdac_hdmi.c > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev) > INIT_LIST_HEAD(&hdmi_priv->pin_list); > INIT_LIST_HEAD(&hdmi_priv->cvt_list); > > + ret = snd_hdac_display_power(edev->hdac.bus, true); > + if (ret < 0) { > + dev_err(&edev->hdac.dev, > + "Cannot turn on display power on i915 err: %d\n", > + ret); > + return ret; > + } > + > ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); > if (ret < 0) { > dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret); > -- > 1.9.1 No counterpart to turn off? Takashi
On Tue, 01 Dec 2015 22:48:41 +0100, Subhransu S. Prusty wrote: > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote: > > On Tue, 01 Dec 2015 18:47:11 +0100, > > Subhransu S. Prusty wrote: > > > > > > From: Ramesh Babu <ramesh.babu@intel.com> > > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > --- > > > sound/soc/codecs/hdac_hdmi.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c > > > index 46aa5cc..fa3cfa5 100644 > > > --- a/sound/soc/codecs/hdac_hdmi.c > > > +++ b/sound/soc/codecs/hdac_hdmi.c > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev) > > > INIT_LIST_HEAD(&hdmi_priv->pin_list); > > > INIT_LIST_HEAD(&hdmi_priv->cvt_list); > > > > > > + ret = snd_hdac_display_power(edev->hdac.bus, true); > > > + if (ret < 0) { > > > + dev_err(&edev->hdac.dev, > > > + "Cannot turn on display power on i915 err: %d\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); > > > if (ret < 0) { > > > dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret); > > > -- > > > 1.9.1 > > > > No counterpart to turn off? > turned off in runtime suspend during first explicit suspend call. It's a refcount, hence it does matter how many times it's called. Does it really balance well? It smells suspicious if you add only a call to turn on without turning off... Takashi
> On Tue, 01 Dec 2015 22:48:41 +0100, > Subhransu S. Prusty wrote: > > > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote: > > > On Tue, 01 Dec 2015 18:47:11 +0100, > > > Subhransu S. Prusty wrote: > > > > > > > > From: Ramesh Babu <ramesh.babu@intel.com> > > > > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > > --- > > > > sound/soc/codecs/hdac_hdmi.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c > b/sound/soc/codecs/hdac_hdmi.c > > > > index 46aa5cc..fa3cfa5 100644 > > > > --- a/sound/soc/codecs/hdac_hdmi.c > > > > +++ b/sound/soc/codecs/hdac_hdmi.c > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct > hdac_ext_device *edev) > > > > INIT_LIST_HEAD(&hdmi_priv->pin_list); > > > > INIT_LIST_HEAD(&hdmi_priv->cvt_list); > > > > > > > > + ret = snd_hdac_display_power(edev->hdac.bus, true); > > > > + if (ret < 0) { > > > > + dev_err(&edev->hdac.dev, > > > > + "Cannot turn on display power on i915 err: %d\n", > > > > + ret); > > > > + return ret; > > > > + } > > > > + > > > > ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); > > > > if (ret < 0) { > > > > dev_err(&codec->dev, "Failed in parse and map nid with err: > %d\n", ret); > > > > -- > > > > 1.9.1 > > > > > > No counterpart to turn off? > > turned off in runtime suspend during first explicit suspend call. > > It's a refcount, hence it does matter how many times it's called. > Does it really balance well? It smells suspicious if you add only a > call to turn on without turning off... Display power is turned ON during device probe. When soc codec probe completes, runtime suspend call is invoked and display power is turned OFF. So refcount is balanced.
On Tue, 01 Dec 2015 17:48:43 +0100, Babu, Ramesh wrote: > > > On Tue, 01 Dec 2015 22:48:41 +0100, > > Subhransu S. Prusty wrote: > > > > > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote: > > > > On Tue, 01 Dec 2015 18:47:11 +0100, > > > > Subhransu S. Prusty wrote: > > > > > > > > > > From: Ramesh Babu <ramesh.babu@intel.com> > > > > > > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > > > --- > > > > > sound/soc/codecs/hdac_hdmi.c | 8 ++++++++ > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c > > b/sound/soc/codecs/hdac_hdmi.c > > > > > index 46aa5cc..fa3cfa5 100644 > > > > > --- a/sound/soc/codecs/hdac_hdmi.c > > > > > +++ b/sound/soc/codecs/hdac_hdmi.c > > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct > > hdac_ext_device *edev) > > > > > INIT_LIST_HEAD(&hdmi_priv->pin_list); > > > > > INIT_LIST_HEAD(&hdmi_priv->cvt_list); > > > > > > > > > > + ret = snd_hdac_display_power(edev->hdac.bus, true); > > > > > + if (ret < 0) { > > > > > + dev_err(&edev->hdac.dev, > > > > > + "Cannot turn on display power on i915 err: %d\n", > > > > > + ret); > > > > > + return ret; > > > > > + } > > > > > + > > > > > ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); > > > > > if (ret < 0) { > > > > > dev_err(&codec->dev, "Failed in parse and map nid with err: > > %d\n", ret); > > > > > -- > > > > > 1.9.1 > > > > > > > > No counterpart to turn off? > > > turned off in runtime suspend during first explicit suspend call. > > > > It's a refcount, hence it does matter how many times it's called. > > Does it really balance well? It smells suspicious if you add only a > > call to turn on without turning off... > > Display power is turned ON during device probe. When soc codec probe > completes, runtime suspend call is invoked and display power is turned OFF. > So refcount is balanced. The runtime suspend of the codec, or of the controller? I haven't seen the codec suspend/resume doing it in this patchset, so I wonder. if it's really well balanced, you should add the proper comment in the code who turns off at when and where. Otherwise reader will be lost. Takashi
On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote: > On Tue, 01 Dec 2015 18:47:11 +0100, > Subhransu S. Prusty wrote: > > > > From: Ramesh Babu <ramesh.babu@intel.com> > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > --- > > sound/soc/codecs/hdac_hdmi.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c > > index 46aa5cc..fa3cfa5 100644 > > --- a/sound/soc/codecs/hdac_hdmi.c > > +++ b/sound/soc/codecs/hdac_hdmi.c > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev) > > INIT_LIST_HEAD(&hdmi_priv->pin_list); > > INIT_LIST_HEAD(&hdmi_priv->cvt_list); > > > > + ret = snd_hdac_display_power(edev->hdac.bus, true); > > + if (ret < 0) { > > + dev_err(&edev->hdac.dev, > > + "Cannot turn on display power on i915 err: %d\n", > > + ret); > > + return ret; > > + } > > + > > ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); > > if (ret < 0) { > > dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret); > > -- > > 1.9.1 > > No counterpart to turn off? turned off in runtime suspend during first explicit suspend call. > > > Takashi
On Wed, 02 Dec 2015 16:37:36 +0100, Subhransu S. Prusty wrote: > > On Tue, Dec 01, 2015 at 06:08:28PM +0100, Takashi Iwai wrote: > > On Tue, 01 Dec 2015 17:48:43 +0100, > > Babu, Ramesh wrote: > > > > > > > On Tue, 01 Dec 2015 22:48:41 +0100, > > > > Subhransu S. Prusty wrote: > > > > > > > > > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote: > > > > > > On Tue, 01 Dec 2015 18:47:11 +0100, > > > > > > Subhransu S. Prusty wrote: > > > > > > > > > > > > > > From: Ramesh Babu <ramesh.babu@intel.com> > > > > > > > > > > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > > > > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > > > > > --- > > > > > > > sound/soc/codecs/hdac_hdmi.c | 8 ++++++++ > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c > > > > b/sound/soc/codecs/hdac_hdmi.c > > > > > > > index 46aa5cc..fa3cfa5 100644 > > > > > > > --- a/sound/soc/codecs/hdac_hdmi.c > > > > > > > +++ b/sound/soc/codecs/hdac_hdmi.c > > > > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct > > > > hdac_ext_device *edev) > > > > > > > INIT_LIST_HEAD(&hdmi_priv->pin_list); > > > > > > > INIT_LIST_HEAD(&hdmi_priv->cvt_list); > > > > > > > > > > > > > > + ret = snd_hdac_display_power(edev->hdac.bus, true); > > > > > > > + if (ret < 0) { > > > > > > > + dev_err(&edev->hdac.dev, > > > > > > > + "Cannot turn on display power on i915 err: %d\n", > > > > > > > + ret); > > > > > > > + return ret; > > > > > > > + } > > > > > > > + > > > > > > > ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); > > > > > > > if (ret < 0) { > > > > > > > dev_err(&codec->dev, "Failed in parse and map nid with err: > > > > %d\n", ret); > > > > > > > -- > > > > > > > 1.9.1 > > > > > > > > > > > > No counterpart to turn off? > > > > > turned off in runtime suspend during first explicit suspend call. > > > > > > > > It's a refcount, hence it does matter how many times it's called. > > > > Does it really balance well? It smells suspicious if you add only a > > > > call to turn on without turning off... > > > > > > Display power is turned ON during device probe. When soc codec probe > > > completes, runtime suspend call is invoked and display power is turned OFF. > > > So refcount is balanced. > > > > The runtime suspend of the codec, or of the controller? I haven't > > seen the codec suspend/resume doing it in this patchset, so I wonder. > > It is turned off in the runtime suspend of the codec. It was added in the > basic driver patch series as part of PM enabling where it enables the > display during runtime resume and disable during runtime suspend. But > when the device runtime PM is first enabled and the device is put to > suspend, only runtime suspend gets called which doesn't actually decrement > the display reference count but throws a warning. With this patch that > gets fixed as well. So, the patch is about fixing the unbalance. It clarifies my suspect. The problem is that $SUBJECT doesn't mention it at all, and even the patch had zero changelog text. Please document more! Also, if this is a fix, you should put Fixes tag. BTW, who will turn off the display power at codec removal? thanks, Takashi > > if it's really well balanced, you should add the proper comment in the > > code who turns off at when and where. Otherwise reader will be lost. > > Sure. Will udpate the changelog and add some comments in the code as well. > > > > > > > Takashi > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > -- >
On Wed, 02 Dec 2015 11:24:53 +0100, Takashi Iwai wrote: > > BTW, who will turn off the display power at codec removal? ... that is, does the driver keep the display off or does it leave on? A common practice for avoiding the unbalance is to add the on/off always as a pair. I guess it applies here, too. Takashi
On Tue, Dec 01, 2015 at 06:08:28PM +0100, Takashi Iwai wrote: > On Tue, 01 Dec 2015 17:48:43 +0100, > Babu, Ramesh wrote: > > > > > On Tue, 01 Dec 2015 22:48:41 +0100, > > > Subhransu S. Prusty wrote: > > > > > > > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote: > > > > > On Tue, 01 Dec 2015 18:47:11 +0100, > > > > > Subhransu S. Prusty wrote: > > > > > > > > > > > > From: Ramesh Babu <ramesh.babu@intel.com> > > > > > > > > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > > > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > > > > --- > > > > > > sound/soc/codecs/hdac_hdmi.c | 8 ++++++++ > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c > > > b/sound/soc/codecs/hdac_hdmi.c > > > > > > index 46aa5cc..fa3cfa5 100644 > > > > > > --- a/sound/soc/codecs/hdac_hdmi.c > > > > > > +++ b/sound/soc/codecs/hdac_hdmi.c > > > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct > > > hdac_ext_device *edev) > > > > > > INIT_LIST_HEAD(&hdmi_priv->pin_list); > > > > > > INIT_LIST_HEAD(&hdmi_priv->cvt_list); > > > > > > > > > > > > + ret = snd_hdac_display_power(edev->hdac.bus, true); > > > > > > + if (ret < 0) { > > > > > > + dev_err(&edev->hdac.dev, > > > > > > + "Cannot turn on display power on i915 err: %d\n", > > > > > > + ret); > > > > > > + return ret; > > > > > > + } > > > > > > + > > > > > > ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); > > > > > > if (ret < 0) { > > > > > > dev_err(&codec->dev, "Failed in parse and map nid with err: > > > %d\n", ret); > > > > > > -- > > > > > > 1.9.1 > > > > > > > > > > No counterpart to turn off? > > > > turned off in runtime suspend during first explicit suspend call. > > > > > > It's a refcount, hence it does matter how many times it's called. > > > Does it really balance well? It smells suspicious if you add only a > > > call to turn on without turning off... > > > > Display power is turned ON during device probe. When soc codec probe > > completes, runtime suspend call is invoked and display power is turned OFF. > > So refcount is balanced. > > The runtime suspend of the codec, or of the controller? I haven't > seen the codec suspend/resume doing it in this patchset, so I wonder. It is turned off in the runtime suspend of the codec. It was added in the basic driver patch series as part of PM enabling where it enables the display during runtime resume and disable during runtime suspend. But when the device runtime PM is first enabled and the device is put to suspend, only runtime suspend gets called which doesn't actually decrement the display reference count but throws a warning. With this patch that gets fixed as well. > > if it's really well balanced, you should add the proper comment in the > code who turns off at when and where. Otherwise reader will be lost. Sure. Will udpate the changelog and add some comments in the code as well. > > > Takashi > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Wed, Dec 02, 2015 at 11:24:53AM +0100, Takashi Iwai wrote: > On Wed, 02 Dec 2015 16:37:36 +0100, > Subhransu S. Prusty wrote: > > > > On Tue, Dec 01, 2015 at 06:08:28PM +0100, Takashi Iwai wrote: > > > On Tue, 01 Dec 2015 17:48:43 +0100, > > > Babu, Ramesh wrote: > > > > > > > > > On Tue, 01 Dec 2015 22:48:41 +0100, > > > > > Subhransu S. Prusty wrote: > > > > > > > > > > > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote: > > > > > > > On Tue, 01 Dec 2015 18:47:11 +0100, > > > > > > > Subhransu S. Prusty wrote: > > > > > > > > > > > > > > > > From: Ramesh Babu <ramesh.babu@intel.com> > > > > > > > > > > > > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > > > > > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > > > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > > > > > > --- > > > > > > > > sound/soc/codecs/hdac_hdmi.c | 8 ++++++++ > > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c > > > > > b/sound/soc/codecs/hdac_hdmi.c > > > > > > > > index 46aa5cc..fa3cfa5 100644 > > > > > > > > --- a/sound/soc/codecs/hdac_hdmi.c > > > > > > > > +++ b/sound/soc/codecs/hdac_hdmi.c > > > > > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct > > > > > hdac_ext_device *edev) > > > > > > > > INIT_LIST_HEAD(&hdmi_priv->pin_list); > > > > > > > > INIT_LIST_HEAD(&hdmi_priv->cvt_list); > > > > > > > > > > > > > > > > + ret = snd_hdac_display_power(edev->hdac.bus, true); > > > > > > > > + if (ret < 0) { > > > > > > > > + dev_err(&edev->hdac.dev, > > > > > > > > + "Cannot turn on display power on i915 err: %d\n", > > > > > > > > + ret); > > > > > > > > + return ret; > > > > > > > > + } > > > > > > > > + > > > > > > > > ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); > > > > > > > > if (ret < 0) { > > > > > > > > dev_err(&codec->dev, "Failed in parse and map nid with err: > > > > > %d\n", ret); > > > > > > > > -- > > > > > > > > 1.9.1 > > > > > > > > > > > > > > No counterpart to turn off? > > > > > > turned off in runtime suspend during first explicit suspend call. > > > > > > > > > > It's a refcount, hence it does matter how many times it's called. > > > > > Does it really balance well? It smells suspicious if you add only a > > > > > call to turn on without turning off... > > > > > > > > Display power is turned ON during device probe. When soc codec probe > > > > completes, runtime suspend call is invoked and display power is turned OFF. > > > > So refcount is balanced. > > > > > > The runtime suspend of the codec, or of the controller? I haven't > > > seen the codec suspend/resume doing it in this patchset, so I wonder. > > > > It is turned off in the runtime suspend of the codec. It was added in the > > basic driver patch series as part of PM enabling where it enables the > > display during runtime resume and disable during runtime suspend. But > > when the device runtime PM is first enabled and the device is put to > > suspend, only runtime suspend gets called which doesn't actually decrement > > the display reference count but throws a warning. With this patch that > > gets fixed as well. > > So, the patch is about fixing the unbalance. It clarifies my > suspect. The problem is that $SUBJECT doesn't mention it at all, and > even the patch had zero changelog text. Please document more! Sorry about that. Will add more in the changelog and add comments in the code as well. > > Also, if this is a fix, you should put Fixes tag. Sure will do that. > > BTW, who will turn off the display power at codec removal? The driver keeps the display off. The display power is on in the RTPM resume when a playbck starts and off in the RTPM suspend callback once the playback is stopped except for the above scenario where it is turned on during dev_probe call and put to off during the first explicit call to runtime suspend. Regards, Subhransu > > > thanks, > > Takashi > > > > if it's really well balanced, you should add the proper comment in the > > > code who turns off at when and where. Otherwise reader will be lost. > > > > Sure. Will udpate the changelog and add some comments in the code as well. > > > > > > > > > > > Takashi > > > _______________________________________________ > > > Alsa-devel mailing list > > > Alsa-devel@alsa-project.org > > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > > > -- > >
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 46aa5cc..fa3cfa5 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev) INIT_LIST_HEAD(&hdmi_priv->pin_list); INIT_LIST_HEAD(&hdmi_priv->cvt_list); + ret = snd_hdac_display_power(edev->hdac.bus, true); + if (ret < 0) { + dev_err(&edev->hdac.dev, + "Cannot turn on display power on i915 err: %d\n", + ret); + return ret; + } + ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais); if (ret < 0) { dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret);