[slawblauciak]

This is in order to delegate the xrun recovery to the driver/ALSA.
Shouldn't be merged until the driver is ready.

Signed-off-by: Slawomir Blauciak slawomir.blauciak@linux.intel.com

[plbossart]

On 12/18/18 6:42 AM, Sławomir Błauciak wrote: This is in order to delegate the xrun recovery to the driver/ALSA. Shouldn't be merged until the driver is ready.

is there a matching PR for the Linux driver?

…

[slawblauciak]

Not yet, we're still in contact with the driver maintainers though.

[plbossart]

On 12/18/18 8:20 AM, Sławomir Błauciak wrote: Not yet, we're still in contact with the driver maintainers though.

I didn't know I had lost my job :-)

…

[keyonjie]

@plbossart I will do a PR for this on driver side.

[keyonjie]

@slawblauciak we will only make sure sending trigger stop/start to FW once Xrun IPC received, please be noticed that is actually similar to Xrun injection, that is actually the issue reported in #506.

So, we may still need fix in FW side.

[wenqingfu]

anchor with #710

[lgirdwood]

Can we also add XRUN_RECOVERY as a status flag in the FW ready message so that we know if it's enabled or not.

[slawblauciak]

Good idea, I will do so

[keyonjie]

@slawblauciak I think we still need trigger XRUN to let all components stop before ALSA stop->prepare->start, otherwise, the dai interrupts will keep arriving and dai_dma_cb() will keep scheduling pipeline_copy().

/* handle pipeline XRUN */
-- static int pipeline_xrun_recover(struct pipeline *p)
++ static int pipeline_xrun_handle(struct pipeline *p)
{
	int ret;

	trace_pipe_error_with_ids(p, "pipeline_xrun_recover()");

	/* notify all pipeline comps we are in XRUN */
	ret = pipeline_trigger(p, p->source_comp, COMP_TRIGGER_XRUN);
	if (ret < 0) {
		trace_pipe_error_with_ids(p, "pipeline_xrun_recover() error: "
					  "Pipelines notification about XRUN "
					  "failed, ret = %d", ret);
		return ret;
	}
	p->xrun_bytes = 0;

#if XRUN_RECOVERY_ENABLED
	/* prepare the pipeline */
	ret = pipeline_prepare(p, p->source_comp);
	if (ret < 0) {
		trace_pipe_error_with_ids(p, "pipeline_xrun_recover() error: "
					  "pipeline_prepare() failed, ret = %d",
					  ret);
		return ret;
	}

	/* restart pipeline comps */
	ret = pipeline_trigger(p, p->source_comp, COMP_TRIGGER_START);
	if (ret < 0) {
		trace_pipe_error_with_ids(p, "pipeline_xrun_recover() error: "
					  "pipeline_trigger() failed, ret = %d",
					  ret);
		return ret;
	}

#endif
	return 0;
}

[keyonjie]

driver side thesofproject/linux#479 is companion with this.

[slawblauciak]

Yeah, that's also a good idea. I will implement it

[plbossart]

@keyonjie Is this just me having lost track of things or is this underflow handling still not resolved? I see no progress be it on firmware or driver sides?

[keyonjie]

@keyonjie Is this just me having lost track of things or is this underflow handling still not resolved? I see no progress be it on firmware or driver sides?

your understand is correct, Pierre, we need both improvement on both FW and driver for this, the effort is paused because of holiday and other higher priority tasks injected.

The latest status on my side is that, with(or without) PRs on FW and driver merged, the FW initiated Xrun can't be recovered and FW can't go back to normal once that happened.

[keyonjie]

@slawblauciak any update for this?

[slawblauciak]

@slawblauciak any update for this?

Is there a PR on the driver side that's ready for merging?

[lgirdwood]

@keyonjie I'd expect driver side PR will only need to read XRUN recovery enabled or not at FW boot time. The driver will not need to do any special handling other that the XRUN handling it does today based on the position and timestamp.

[slawblauciak]

@lgirdwood I've extended FW ready message as you suggested

[lgirdwood]

@slawblauciak this name change will need driver update too, not sure if it's too late atm @plbossart ? We can could apply to FW without the name change for the moment

[lgirdwood]

@slawblauciak do you have a small PR for the kernel yet with the same change ? Should be trivial to merge them both together.

[plbossart]

is there a clear behavior on the position of a new bitfield? will this result in this field being added on top, or all fields being moved by one, which is a non-compatible move. A compatible move would be to declare a new structure with the new field in the union.

[slawblauciak]

You're right. The way the compiler lays out the bitfields is not defined, this has to be fixed.

[slawblauciak]

@lgirdwood I think such change should be made in the PR introducing driver-side xrun handling, since it's a required for this to work anyway.

[slawblauciak]

@plbossart I've been thinking about this and I came to a conclusion that adding a separate bitfield is not a good solution either. While it will retain backwards compatibility it will again break whenever somebody decides to add another flag to this field. What about using standard bitshifts for a dedicated caps uint32_t field with explicit ordering? That way we can guarantee compatibility in the long run, even when somebody adds a new flag.

[plbossart]

@slawblauciak yes that sounds like a good idea, thanks for the suggestion. we've already removed enums, let's remove implicit bitfields as well from the IPC.
Using definitions as below would be quite nice.

#define IPC_INFO_BUILD BIT(0)
#define IPC_INFO_LOCKS BIT(1)
#define IPC_INFO_LOCKS_VERBOSE BIT(2)
#define IPC_INFO_GDB BIT(3)
#define IPC_INFO_NO_XRUN_RECOVERY BIT(4)

[slawblauciak]

@plbossart I've made the PRs for this change
#1418
thesofproject/linux#937

[keyonjie]

so less debug .bits set is our intention for DEBUG_BUILD?

[slawblauciak]

The debug bits stay unchanged

[keyonjie]

The debug bits stay unchanged

I meant comparing to non-DEBUG_BUILD(or called normal build), why we assign less debug .bits for DEBUG_BUILD? we assign/initialize .build, .locks, .locks_verbos and .gdb for non-DEBUG_BUILD, but only .gdb is initialized for DEBUG_BUILD, is this our intention?

I know that is not introduced by your PR, I am only curious about the difference here. @lgirdwood any comment?

[lgirdwood]

@keyonjie I think it's just the way the diff is being displayed the macros is correct.

[keyonjie]

others looks good to me.

[plbossart]

Can we clarify where the new bitfield is added? If this moves the position of the previous ones then it's not a minor ABI change?

[plbossart]

this wording is award "this flag disables... should remain enabled". "outside of firmware" isn't very nice either

[plbossart]

is there a clear behavior on the position of a new bitfield? will this result in this field being added on top, or all fields being moved by one, which is a non-compatible move. A compatible move would be to declare a new structure with the new field in the union.

[keyonjie]

To speed up testing/debugging/enabling xrun recovery features, @lgirdwood to me, please feel free to merge this PR.
Actually, both with or without this PR merged, FW initiated Xrun can't recover in the latest codes, I am working on that these days and already have some improvements, I will submit patches based on this one is merged.

[lgirdwood]

@keyonjie this will be merged once the IPC part is aligned with the kernel. i.e. we change name for both or dont change the structure name. @slawblauciak any update on this ?

[slawblauciak]

@lgirdwood matters are not as simple as this change doesn't guarantee backwards compatiblity. C standard does not define the order of the bitfields. I proposed a solution in the discussion with @plbossart

[keyonjie]

can we do this in 2 steps/PRs:

1. add NO_XRUN_RECOVERY marco and cleanup to make sure xrun recovery is thoroughly disabled at that macro.
2. add no_xrun_recovery flag to fw_ready message, bump ABI version, and send kernel PR together.

[keyonjie]

@slawblauciak @lgirdwood let me send a PR for 1) to speed up the process.

[keyonjie]

Here it is: #833

[plbossart]

@slawblauciak this PR seems stale and possibly no longer relevant, can we close it?