| Message ID | 1457606694-10985-1-git-send-email-ckeepax@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7c9190f7e7428487cd67839f9a547efcc9ec3b9b |
| Headers | show |
On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote: > The soc_compr_pointer does not correctly pass any errors returned by the > driver callback back up the stack. This patch corrects this issue. Should we do that :) I am not too sure. Pointer query is supposed to read the current value and return. You are trying to indicate that stream has gone bad which is not the same as read faced an error... Also please use cover letter for these things to describe problem you are trying to solve. > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> > --- > sound/soc/soc-compress.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c > index 875733c..d2df46c 100644 > --- a/sound/soc/soc-compress.c > +++ b/sound/soc/soc-compress.c > @@ -530,14 +530,15 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream, > { > struct snd_soc_pcm_runtime *rtd = cstream->private_data; > struct snd_soc_platform *platform = rtd->platform; > + int ret = 0; > > mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass); > > if (platform->driver->compr_ops && platform->driver->compr_ops->pointer) > - platform->driver->compr_ops->pointer(cstream, tstamp); > + ret = platform->driver->compr_ops->pointer(cstream, tstamp); > > mutex_unlock(&rtd->pcm_mutex); > - return 0; > + return ret; > } > > static int soc_compr_copy(struct snd_compr_stream *cstream, > -- > 2.1.4 >
On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote: > On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote: > > The soc_compr_pointer does not correctly pass any errors returned by the > > driver callback back up the stack. This patch corrects this issue. > > Should we do that :) I am not too sure. Pointer query is supposed to read > the current value and return. You are trying to indicate that stream has > gone bad which is not the same as read faced an error... > > Also please use cover letter for these things to describe problem you are > trying to solve. Apologies for not doing so, I had been viewing this as more of a simple oversight in the framework rather than a design choice. The problem I am looking at is the DSP suffers an unrecoverable error. We can find out about this error in our driver because the DSP returns some error status to us. This is fine if user-space is doing a read as reads return error status back to user-space so the user can find out that things have gone bad. However, if user-space is doing an avail request there is no path for the error to come back up to user-space. The pointer request returns zero available data, so a read never happens and we basically just end up sitting waiting for data on a stream that we know full well has died. I will have a look at other options for propagating this error, I guess it should also be possible to push it through the poll so I can look at why that isn't happening. I guess my return question would be I can imagine many reasons why a pointer query might fail, especially for off chip DSPs. What is the reasoning for wanting to hide those errors from the rest of the system? It seems to me it would be best to handle an error as soon as it is noticed, and if a particular system has a pointer request that never fails then it can just not return an error. Thanks, Charles
On Fri, 11 Mar 2016 11:04:25 +0100, Charles Keepax wrote: > > On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote: > > On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote: > > > The soc_compr_pointer does not correctly pass any errors returned by the > > > driver callback back up the stack. This patch corrects this issue. > > > > Should we do that :) I am not too sure. Pointer query is supposed to read > > the current value and return. You are trying to indicate that stream has > > gone bad which is not the same as read faced an error... > > > > Also please use cover letter for these things to describe problem you are > > trying to solve. > > Apologies for not doing so, I had been viewing this as more of a > simple oversight in the framework rather than a design choice. > > The problem I am looking at is the DSP suffers an unrecoverable > error. We can find out about this error in our driver because the > DSP returns some error status to us. This is fine if user-space > is doing a read as reads return error status back to user-space > so the user can find out that things have gone bad. However, if > user-space is doing an avail request there is no path for the > error to come back up to user-space. The pointer request returns > zero available data, so a read never happens and we basically > just end up sitting waiting for data on a stream that we know > full well has died. > > I will have a look at other options for propagating this error, > I guess it should also be possible to push it through the poll so > I can look at why that isn't happening. > > I guess my return question would be I can imagine many reasons > why a pointer query might fail, especially for off chip DSPs. > What is the reasoning for wanting to hide those errors from the > rest of the system? It seems to me it would be best to handle an > error as soon as it is noticed, and if a particular system has a > pointer request that never fails then it can just not return an > error. IMO, propagating the error immediately is a good thing. I guess it wasn't checked in the pointer callback just because the pointer callback was supposed to be a simple state copy without involving the state change. OTOH, another question is whether it's enough just to tell the error there as is. When such an error is detected, it essentially means that the whole DSP got wrong. What we can do at best is to prepare for recovery. And this requires the state change. Actually, PCM pointer callback may return a special value indicating an XRUN error. The PCM core reacts for it, stops the stream and changes the stream state, so that further accesses get the error consistently. Similar mechanism would be needed for compress API, I suppose. Takashi
On Fri, Mar 11, 2016 at 11:25:22AM +0100, Takashi Iwai wrote: > > I guess my return question would be I can imagine many reasons > > why a pointer query might fail, especially for off chip DSPs. > > What is the reasoning for wanting to hide those errors from the > > rest of the system? It seems to me it would be best to handle an > > error as soon as it is noticed, and if a particular system has a > > pointer request that never fails then it can just not return an > > error. > > IMO, propagating the error immediately is a good thing. I guess it > wasn't checked in the pointer callback just because the pointer > callback was supposed to be a simple state copy without involving the > state change. > > OTOH, another question is whether it's enough just to tell the error > there as is. When such an error is detected, it essentially means > that the whole DSP got wrong. What we can do at best is to prepare > for recovery. And this requires the state change. Yes, for recovery on our DSP we do inoke snd_pcm_stop() for all the open streams and then get these restarted. I think we need to do same for compress too.. > Actually, PCM pointer callback may return a special value indicating > an XRUN error. The PCM core reacts for it, stops the stream and > changes the stream state, so that further accesses get the error > consistently. Similar mechanism would be needed for compress API, I > suppose. I didnt know that :)
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index 875733c..d2df46c 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -530,14 +530,15 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream, { struct snd_soc_pcm_runtime *rtd = cstream->private_data; struct snd_soc_platform *platform = rtd->platform; + int ret = 0; mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass); if (platform->driver->compr_ops && platform->driver->compr_ops->pointer) - platform->driver->compr_ops->pointer(cstream, tstamp); + ret = platform->driver->compr_ops->pointer(cstream, tstamp); mutex_unlock(&rtd->pcm_mutex); - return 0; + return ret; } static int soc_compr_copy(struct snd_compr_stream *cstream,
The soc_compr_pointer does not correctly pass any errors returned by the driver callback back up the stack. This patch corrects this issue. Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> --- sound/soc/soc-compress.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)