Message-ID: <Y87DNf1tnB8aaKJS@orome>
Date:   Mon, 23 Jan 2023 18:26:13 +0100
From:   Thierry Reding <treding@...dia.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Mohan Kumar D <mkumard@...dia.com>, will@...nel.org,
        dmitry.baryshkov@...aro.org, shawnguo@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        jonathanh@...dia.com
Subject: Re: [PATCH v2] arm64: defconfig: Enable HDA INTEL config for ARM64

On Mon, Jan 23, 2023 at 05:05:08PM +0100, Krzysztof Kozlowski wrote:
> On 23/01/2023 16:58, Thierry Reding wrote:
> > On Fri, Jan 20, 2023 at 06:00:25PM +0100, Krzysztof Kozlowski wrote:
> >> On 20/01/2023 17:56, Catalin Marinas wrote:
> >>> On Fri, Jan 20, 2023 at 07:20:01AM +0100, Krzysztof Kozlowski wrote:
> >>>> On 20/01/2023 06:48, Mohan Kumar D wrote:
> >>>>> On 18-01-2023 18:06, Krzysztof Kozlowski wrote:
> >>>>>> External email: Use caution opening links or attachments
> >>>>>> On 18/01/2023 12:46, Mohan Kumar D wrote:
> >>>>>>> On 18-01-2023 13:04, Krzysztof Kozlowski wrote:
> >>>>>>>> External email: Use caution opening links or attachments
> >>>>>>>> On 17/01/2023 19:16, Mohan Kumar wrote:
> >>>>>>>>> Enable CONFIG_SND_HDA_INTEL for NVIDIA PCI based graphics sound card for
> >>>>>>>>> ARM64 based platforms as Intel PCI driver was used for registering the
> >>>>>>>>> sound card.
> >>>>>>>> It's not a part of SoC, not a common device used during debugging or
> >>>>>>>> development, so I don't think it is reasonable to enable it. We do not
> >>>>>>>> enable driver just because someone uses them. Otherwise please clarify
> >>>>>>>> which board has this device embedded (not pluggable by user, but embedded).
> >>>>>>> This change is required for enabling HDA sound registration for Nvidia
> >>>>>>> discrete GPU cards based on PCI and pluggable to Nvidia Jetson Platforms.
> >>>>>> You can plug anything to PCI slot and we do not enable every such PCI
> >>>>>> adapter.
> >>>>> Without this config enabled, the Intel hda audio driver won't be built 
> >>>>> and dGPU won't be able to register sound card. Do you have any 
> >>>>> suggestion here?
> >>>>
> >>>> Without hundreds of other drivers they also won't be built and won't be
> >>>> usable. Anyway, this is just defconfig, so it does not matter. You can
> >>>> always enable it in your setup, why is this a problem?
> >>>>
> >>>> Again, we do not enable drivers for every PCI card.
> >>>
> >>> I don't think we have any set rules for what goes in a defconfig. If one
> >>> has a real use-case, we tend to enable stuff in defconfig, especially if
> >>> it's a module.
> >>
> >> There will be always an use case for every PCI and USB card. It's not
> >> related to storage or networking which could justify device bringup
> >> (rootfs). It's really not needed for any board operation. defconfig is
> >> not for marketing products but for our development and reference platforms.
> > 
> > If defconfig were only for boot-critical drivers, it's terribly bloated
> 
> We enable drivers for devices present in our platforms. Everything which
> is on such platforms. For pluggable USB/PCI/whatever third-party
> devices, then comes the argument as boot-related.
> 
> > as it is. We enable things like multimedia, infrared and audio. None of
> > those are critical to booting a system. Heck, we also enable most of
> > DRM/KMS, which are useful for boot on consumer devices, but are rarely
> > critical on development and reference platforms.
> > 
> > Besides, a PCI board can be considered a development platform depending
> > on who you are.
> > 
> > I've always looked at defconfig as more of a guideline as to what's a
> > useful baseline configuration for an architecture.
> 
> Yep and this one here is nowhere near that architecture. It's pluggable
> card, not related to hardware nor arm64 (If I understood correctly). Why
> you do not enable it on x86? Or multi_v7? or hundreds of other defconfigs?

My understanding is that the code behind SND_HDA_INTEL, despite the
name, is in fact generic PCI HDA code and is compatible with a number of
non-Intel devices. It's also nothing x86-specific because this code
works on any PCI-enabled platform (it's for driving generic PCI HDA
controllers).

It's quite common for people to hook up PCI cards on NVIDIA Jetson
platforms (which are arm64). Often these will be NVIDIA GPUs, but they
can be any brand of GPU, really (SND_HDA_INTEL supports hardware from a
large number of vendors).

> >> The only argument behind this change is "I have a PCI card and I want it
> >> in defconfig", but why it has to be in defconfig in the first place?
> >> There is no reason. This is not distro...
> > 
> > That's highly subjective and honestly that argument can go in both
> > directions. People can, after all, start from an allnoconfig and then
> > work their way up to something that's usable on their particular device.
> > Or they could start from an allmodconfig and work their way down.
> 
> I am sorry, but adding new stuff does not require arguments against.
> Adding new stuff requires argument for it. You reverse the argumentation
> that I need to find proves that we do not need it in mainline platforms,
> if I got your response correctly.

Adding new stuff also doesn't require arguments in favour. At least
that's not been my experience in kernel development. If somebody spends
the time to write a driver, we don't tend to ask proof of why it's
useful, we trust that they wouldn't have spent the effort if they didn't
think it worth it.

Besides, this is to enable support for these various HDA controllers
that can be plugged into Jetson platforms, which very well are mainline
platforms.

> > The point of defconfig is to give you something that's somewhere between
> > the two extremes. Obviously if we start enabling everything, it defeats
> > that purpose. If we prohibit the enablement of new options, we equally
> > limit its usefulness.
> 
> I don't think we discuss the same thing. There are no extremes here at
> all. The patch is about enabling arm64-unrelated PCI pluggable device,
> just because it came from @nvidia.com author. If you think some PCI

I think you're making this into something it's not. What would be the
benefit for NVIDIA to enable something that is unused? If there's
consensus that only boot-critical pieces should go into defconfig, then
that's fine with me. Currently that's not the case, so painting this as
some sort of marketing stunt is hardly fair.

> pluggable 3rd party device is suitable for defconfig, I will bring
> hundreds of other drivers I am also plugging over PCI to my boards, just
> because I want some audio.
> 
> It's not reasonable path...

Well, then what is a reasonable path? We've had per-platform defconfigs
on 32-bit ARM and people weren't happy about those. But if the single
defconfig on 64-bit ARM doesn't generate a configuration that is useful
for many people, then we don't need it. People could then just keep
their own, local default configurations.

Surely I'm not the only one that gets annoyed if I have to tweak config
and rebuild multiple times until I have the right configuration.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists