[Sound-open-firmware] can stream-box size be 0?

[Guennadi Liakhovetski]

On Wed, Apr 03, 2019 at 05:41:31PM +0200, Guennadi Liakhovetski wrote:
> On Wed, Apr 03, 2019 at 04:22:42PM +0100, Liam Girdwood wrote:
> > On Wed, 2019-04-03 at 17:18 +0200, Guennadi Liakhovetski wrote:
> > > On Wed, Apr 03, 2019 at 03:56:22PM +0100, Liam Girdwood wrote:
> > > > On Wed, 2019-04-03 at 16:49 +0200, Guennadi Liakhovetski wrote:
> > > > > On Wed, Apr 03, 2019 at 03:39:02PM +0100, Liam Girdwood wrote:
> > > > > > On Wed, 2019-04-03 at 15:33 +0200, Guennadi Liakhovetski wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > In kernel ipc.c I see "if (sdev->stream_box.size == 0)" checks at
> > > > > > > exactly 4 locations, but all currently supported by SOF platforms set
> > > > > > > the size to 0x200. Do we need those checks or can we safely assume,
> > > > > > > that
> > > > > > > the size is never 0?
> > > > > > 
> > > > > > The size can depend on FW platform and how much space they allocate for
> > > > > > mmap()
> > > > > > stream position information. i.e. ALSA can read pipeline stream position
> > > > > > stats
> > > > > > directly from mailbox rather than via IPC.
> > > > > 
> > > > > Does that mean, that I misunderstood something? Could you point me out
> > > > > to the code, that can set that size to anything other than 0x200, as
> > > > > supplied by the firmware?
> > > > > 
> > > > 
> > > > This size can be anything, even 0 is valid. Look at the extended boot data
> > > > sent
> > > > by the FW at boot in platform.c (e.g. src/platform/baytrail/platform.c)
> > > 
> > > Ok, right, I understand that. BTW, a correction: it's actually 0x1000
> > > (4096) on cAVS platforms. But anyway: I understand that it's
> > > configurable by the firmware and at least in theory it's allowed to be
> > > 0. But do we have any platforms where it is 0? I don't see any. Have we
> > > ever tested such a case? The kernel has code that checks it for 0 (see
> > > above), but if we don't have any such platforms it means, those paths
> > > are never entered and never tested. Looking at them their handling also
> > > doesn't seem entirely clear to me. So my question is: do we really want
> > > and need to be able at least in theory to use 0 there and keep that code
> > > and hope, that it works and never breaks, or can we remove that code and
> > > put a check and an failure if it's 0?
> > > 
> > 
> > Keep the check, some platforms may not have a large enough mailbox and depend on
> > IPC for all stream position updates.
> 
> Ok, I'll try to test the 0-size case.

Tested, works. But I still don't understand a couple of things about
this specific IPC.

The firmware sends a position update per

	mailbox_stream_write(cdev->pipeline->posn_offset, posn, sizeof(*posn));
	return ipc_queue_host_message(_ipc, posn->rhdr.hdr.cmd, posn,
				      sizeof(*posn), 0);

I.e. it unconditionally always does both: writes a position struct to
the stream mailbox and sends it via IPC (which usually translates to
writing to the Host Mailbox). So in the firrmware we hard-code, that all
platforms have space in their stream mailbox for position updates.

My question to this is - why always do both, isn't it redundant?

The kernel does

static void ipc_period_elapsed(struct snd_sof_dev *sdev, u32 msg_id)
{
	...

	/* check if we have stream box */
	if (sdev->stream_box.size == 0) {
		/* read back full message */
		snd_sof_ipc_read(sdev, &posn, sizeof(posn));

		spcm = snd_sof_find_spcm_comp(sdev, posn.comp_id, &direction);
	} else {
		spcm = snd_sof_find_spcm_comp(sdev, msg_id, &direction);
	}

which first of all is absolutely theoretical - the SOF *always* writes
position updates to the stream mailbox and all platforms have it,
secondly - why does it use posn.comp_id in the "true" branch instead of
msg_id as in the "false" branch? Can those IDs be different?

Thanks
Guennadi