Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

copier: add support for windows sample type setting #8456

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

RanderWang
Copy link
Collaborator

Windows driver always set sample type of MSB for 24/32 format but SOF FW supports 24/32 LSB type for non-copier modules. So FW will convert the input MSB 24/32 to LSB 24/32 and process it and convert it back to MSB 24/32 when exiting gtw.

@RanderWang
Copy link
Collaborator Author

@dnikodem can you help to check this PR with windows ? thanks

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang does this mean we need to do extra or differet conversions for windows in the copier ? i.e. no net gain in MCPS.

@RanderWang
Copy link
Collaborator Author

@RanderWang does this mean we need to do extra or differet conversions for windows in the copier ? i.e. no net gain in MCPS.

As you know, Windows has a different layout of 24/32 compared to SOF implementation. Windows always set MSB_TYPE all the modules while Linux topology set correct type for each module, so we need to adjust format to choose the correct conversion for this case. It will not affect performance since the conversion function is the same as Linux but the sample type is not fit for SOF.

@lgirdwood
Copy link
Member

@RanderWang does this mean we need to do extra or differet conversions for windows in the copier ? i.e. no net gain in MCPS.

As you know, Windows has a different layout of 24/32 compared to SOF implementation. Windows always set MSB_TYPE all the modules while Linux topology set correct type for each module, so we need to adjust format to choose the correct conversion for this case. It will not affect performance since the conversion function is the same as Linux but the sample type is not fit for SOF.

Thank you for clarifying - just a different conversion in FW with no MCPS impact. Lets wait on @dnikodem review now.

src/audio/copier/copier_generic.c Show resolved Hide resolved
src/audio/copier/copier_generic.c Show resolved Hide resolved
@RanderWang
Copy link
Collaborator Author

Thanks for @dnikodem's hard work on validation on windows platforms. Pass the test, so it is ready for review.

@RanderWang RanderWang changed the title [NOT REVIEW] copier: add support for windows driver copier: add support for windows sample type setting Nov 17, 2023
Windows driver always set sample type of MSB for 24/32 format but SOF FW
supports 24/32 LSB type for non-copier modules. So FW will convert the
input MSB 24/32 to LSB 24/32 and process it and convert it back to MSB
24/32 when exiting gtw.

Signed-off-by: Rander Wang <[email protected]>
Signed-off-by: Damian Nikodem <[email protected]>
Copy link
Contributor

@dnikodem dnikodem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lgirdwood
Copy link
Member

Seeing some cluster failures with nocodec, lets be sure.

@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood lgirdwood added this to the v2.9 milestone Nov 23, 2023
@RanderWang
Copy link
Collaborator Author

Multicore failure. It will be fixed by #8515

@lgirdwood
Copy link
Member

Multicore failure. It will be fixed by #8515

Now merged - will rerun the CI.

@lgirdwood
Copy link
Member

SOFCI TEST

1 similar comment
@RanderWang
Copy link
Collaborator Author

SOFCI TEST

@RanderWang
Copy link
Collaborator Author

RanderWang commented Nov 27, 2023

ignore sof-ci/jenkins/pr-device-test/main-cavs2.5 which is a old test item for old PR and not used by latest PR in Github.
The failure case sof-ci/jenkins/pr-device-test/main-cavs : multiple-pause-resume-50.sh and check-suspend-resume-with-capture.sh are not related to this PR

default:
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we combine two switch case to one and move two if() inside case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets take that as an optimization for followup in another PR.

default:
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets take that as an optimization for followup in another PR.

@lgirdwood lgirdwood merged commit 383d17a into thesofproject:main Nov 27, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants