[Sound-open-firmware] can stream-box size be 0?
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?
Thanks Guennadi
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.
Liam
Thanks Guennadi _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
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?
Thanks Guennadi
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)
Liam
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?
Thanks Guennadi
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.
Liam
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.
Thanks Guennadi
On 2019/4/3 下午9:33, 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?
I think before Xiuli implemented this stream-box(use for stream specific information sharing b/w FW/driver, e.g. position update), this stream_box.size was always 0(stream position was located in dsp-box in this scenario), we added those checks for compatibility purpose on driver side.
I think better to keep checking, but it would be good if we can simplify the checking in ipc_period_elapsed() and ipc_xrun(), e.g. only check once in each function.
Thanks, ~Keyon
Thanks Guennadi _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
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
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.
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
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
On Thu, 2019-04-04 at 17:20 +0200, Guennadi Liakhovetski wrote:
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.
Agree, the naming also implies some HW coupling (but it is a FW concept). It would be good to move this into platform code but it would need to be done with an option to get host DMA position (default) or all position data (full structure with DAI endpoint position and timestamps too). The trace API would need to use this call too for trace position IIRC.
Liam
participants (3)
-
Guennadi Liakhovetski -
Keyon Jie -
Liam Girdwood