Re: [alsa-devel] [PATCH 7/7] ASoC: SOF: Intel: Account for compress streams when servicing IRQs

From: Takashi Iwai <tiwai@suse.de>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: lgirdwood@gmail.com, alsa-devel@alsa-project.org,
	broonie@kernel.org, tiwai@suse.com,
	pierre-louis.bossart@linux.intel.com
Subject: Re: [alsa-devel] [PATCH 7/7] ASoC: SOF: Intel: Account for compress streams when servicing IRQs
Date: Tue, 17 Dec 2019 11:30:51 +0100	[thread overview]
Message-ID: <s5hy2vb1bxg.wl-tiwai@suse.de> (raw)
In-Reply-To: <20191217095851.19629-8-cezary.rojewski@intel.com>

On Tue, 17 Dec 2019 10:58:51 +0100,
Cezary Rojewski wrote:
> 
> Update stream irq handler definition to correctly set hdac_stream
> current position when servicing stream interrupts for compress streams.
> 
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/hdaudio.h          |  1 +
>  sound/soc/sof/intel/hda-stream.c | 26 ++++++++++++++++++++++++--
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 9a8bf1eb7d69..9a24d57f0cf2 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -496,6 +496,7 @@ struct hdac_stream {
>  	bool locked:1;
>  	bool stripe:1;			/* apply stripe control */
>  
> +	unsigned long curr_pos;

Unless the actual implementation of this user is shown, it's a bit
hard to judge whether this addition is really OK or not....

I don't believe it'd be a big problem at all, but still we need the
usage implementation for a proper evaluation.

And, IMO, such a change should be split to two parts: the common API
change and its user.

thanks,

Takashi

>  	/* timestamp */
>  	unsigned long start_wallclk;	/* start + minimum wallclk */
>  	unsigned long period_wallclk;	/* wallclk for period */
> diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
> index c0ab9bb2a797..ddf755a5a730 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -571,6 +571,23 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev)
>  	return ret;
>  }
>  
> +static void hda_dsp_update_bytes_transferred(struct hdac_stream *hstream,
> +					     u64 buffer_size)
> +{
> +	unsigned int prev_pos;
> +	int pos, num_bytes;
> +
> +	div_u64_rem(hstream->curr_pos, buffer_size, &prev_pos);
> +	pos = snd_hdac_stream_get_pos_posbuf(hstream);
> +
> +	if (pos < prev_pos)
> +		num_bytes = (buffer_size - prev_pos) +  pos;
> +	else
> +		num_bytes = pos - prev_pos;
> +
> +	hstream->curr_pos += num_bytes;
> +}
> +
>  static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
>  {
>  	struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
> @@ -588,14 +605,19 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
>  			snd_hdac_stream_writeb(s, SD_STS, sd_status);
>  
>  			active = true;
> -			if (!s->substream ||
> +			if ((!s->substream && !s->cstream) ||
>  			    !s->running ||
>  			    (sd_status & SOF_HDA_CL_DMA_SD_INT_COMPLETE) == 0)
>  				continue;
>  
>  			/* Inform ALSA only in case not do that with IPC */
> -			if (sof_hda->no_ipc_position)
> +			if (s->substream && sof_hda->no_ipc_position) {
>  				snd_sof_pcm_period_elapsed(s->substream);
> +			} else if (s->cstream) {
> +				hda_dsp_update_bytes_transferred(s,
> +					s->cstream->runtime->buffer_size);
> +				snd_compr_fragment_elapsed(s->cstream);
> +			}
>  		}
>  	}
>  
> -- 
> 2.17.1
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel