Re: [PATCH v4 1/2] ALSA: pcm: reinvent the stream synchronization ID API

[Jaroslav Kysela <perex@xxxxxxxx>]

On 13. 06. 24 14:56, Takashi Sakamoto wrote:
> Hi,
>
> On Tue, May 07, 2024 at 03:30:50PM +0200, Jaroslav Kysela wrote:
> > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> > index 628d46a0da92..c458172b32d5 100644
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image {
> >    *                                                                           *
> >    *****************************************************************************/
> >   
> > -#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 17)
> > +#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 18)
> >   
> >   typedef unsigned long snd_pcm_uframes_t;
> >   typedef signed long snd_pcm_sframes_t;
> > @@ -330,12 +330,6 @@ enum {
> >   #endif
> >   };
> >   
> > -union snd_pcm_sync_id {
> > -	unsigned char id[16];
> > -	unsigned short id16[8];
> > -	unsigned int id32[4];
> > -};
>
> It can bring FTBFS for any userspace application which uses the
> structure. If getting rid of such public structure, we should have the
> term to deprecate it for a while.

Marked with __attribute__((deprecated)) in v5.

> > +/**
> > + * is sync id (clock id) empty?
> > + */
> > +static inline bool pcm_sync_empty(const unsigned char *sync)
> > +{
> > +	return get_unaligned((__u64 *)(sync + 0)) == 0 && get_unaligned((__u64 *)(sync + 8)) == 0;
> > +}
>
> This kind of usage of C inline qualifier is not useless, since the modern
> compilers can optimize it into the local code.

Removed get_unaligned() calls. Using standard C code because this code is 
removed in the second patch.

> > +static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream,
> > +				     void *arg)
> > +{
> > +	struct snd_pcm_hw_params *params = arg;
> > +
> > +	if (pcm_sync_empty(params->sync))
> > +		memcpy(params->sync, substream->runtime->sync, sizeof(params->sync));
> > +	return 0;
> > +}
>
> Furthermore, it does not work as expected in the case that any of driver sets
> zeros to char[16] intentionally for its own purpose at PCM .open callback.
>
> I think it is a kind of 'Separation of mechanism and policy' argument,
> and a kind of designs which should be avoided.

The second patch resolves this.

> > -	runtime->sync.id32[0] = substream->pcm->card->number;
> > -	runtime->sync.id32[1] = 'P';
> > -	runtime->sync.id32[2] = 16;
> > -	runtime->sync.id32[3] = 'V';
> > +	*(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
> > +	memset(runtime->sync + 4, 0, sizeof(runtime->sync) - 4);
> > +	strncpy(runtime->sync + 4, "P16V", 4);
>
> I'm sorry if I overlooked your reply to my point, however the above is
> not equivalent.

It does not matter. The ID should not be used for a direct comparison. The ID 
must be only unique for the system and applications should compare those IDs 
between PCM streams (including multiple sound cards). If equal - the source 
clock source is equal. I tried to explain this in the commit message in v5.

				Thanks for your review,
						Jaroslav

--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.