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

[Guennadi Liakhovetski]

On Thu, Apr 04, 2019 at 02:18:32PM +0100, Liam Girdwood wrote:
> On Thu, 2019-04-04 at 14:42 +0200, Guennadi Liakhovetski wrote:
> > 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?
> > 
> You are assuming that SOF will only ever run on a single platform. Some
> platforms have HW registers to propagate stream position, some have shared
> memory, some have neither hence the need to support position readback via
> various methods.

Actually I'm trying to do the opposite - I'm trying to move all
platform-specific code out of the core to make the code easier to port.
And the very check above "if (sdev->stream_box.size == 0)" seems quite
hardware specific to me. When we do get platforms where we cannot use
streaming buffers to also transfer stream position information, I think
chances are, that we anyway will have to modify this and other parts
quite a bit.

But nm, I'll keep this check for now, I'll just reorganise the code a
bit.

Thanks
Guennadi

> On SKL onwards preferred method is HDA stream position register, but this not
> give full positions for every endpoint so we can also support mmap readback here
> too.
> 
> Liam
> 
>