Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control

From: Arnaud Pouliquen <arnaud.pouliquen@st.com>
To: Takashi Sakamoto <o-takashi@sakamocchi.jp>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Cc: "kernel@stlinux.com" <kernel@stlinux.com>,
	Takashi Iwai <tiwai@suse.de>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	Jyri Sarha <jsarha@ti.com>, David Airlie <airlied@linux.ie>,
	"broonie@kernel.org" <broonie@kernel.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 2/2] ASoC: hdmi-codec: add channel mapping control
Date: Tue, 13 Dec 2016 15:49:49 +0100	[thread overview]
Message-ID: <3943870f-c98b-da0e-e4e7-0a4a3fd337c7@st.com> (raw)
In-Reply-To: <d8b4e5cf-f297-f075-801f-6517393d9ca7@sakamocchi.jp>

On 12/13/2016 02:58 PM, Takashi Sakamoto wrote:
> On 2016年12月13日 22:23, Takashi Sakamoto wrote:
>> Hi Arnaud,
>>>> This table is invariant in lifetime of the storage object, as well.
>>>> Let's put into .rodata section, too.
>>>>
>>> This table is updated in hdmi_codec_cea_init_channel_alloc so can not be
>>> constant. In theory i could declare all field instead of computing some.
>>> But for lisibility, i would prefer to just declare ca_index  and
>>> speakers allocation field in this table (i will declared both as const)
>>
>> You should pay enough attention to a case that one system has several
>> GPUs to which relevant GPU drivers register HDMI_CODEC_DRV_NAME platform
>> device. The 'static' modifier has an effect to keep just one storage
>> object, thus your code causes bugs in the case.
> 
> Oops, the bug is unrelated to the static modifier. The modifier is for 
> reference scope. I'll correct as the file local symbol has just one 
> storage object.
> 
> (I might be tired tonight...)
> 
i understood in this way :-)
As 'ca_index' and "speakers" fiel are constants
the fields computed in hdmi_codec_cea_init_channel_alloc will
not change if the functions is called several time ( multi-instances).
but agree that it is not very clean, but this should work.
To avoid to compute several time, i can also add a test in
hdmi_codec_cea_init_channel_alloc to do it only one time (by testing
hdmi_codec_channel_alloc[0].channels), or perhaps better an atomic local
variable to avoid side effect for Multiprocessor archs.

Now the second approach is to define all the field. This indeeds a big
table (around 256 lines instead of 32 lines):

static const struct hdmi_codec_cea_spk_alloc hdmi_codec_channel_alloc[] = {
	{
		.ca_index = 0x00,
		.speakers = {   0,    0,   0,   0,   0,    0,  FR,  FL } },
		.channels = 2,
		.spk_mask = FR | FL,
		.spk_na_mask = 0x00,
	},
	{ /* 2.1 */
		.ca_index = 0x01,
		.speakers = {   0,    0,   0,   0,   0,  LFE,  FR,  FL } },
		.channels = 3,
		.spk_mask = FR | FL | LFE,
		.spk_na_mask = 0x00,
	},
	{ /* Dolby Surround */
		.ca_index = 0x02,
		.speakers = {   0,    0,   0,   0,  FC,    0,  FR,  FL } },
		.channels = 4,
		.spk_mask = FR | FL | FC,
		.spk_na_mask = BIT(2),
	},
[...]

	{
		.ca_index = 0x1f,
		.speakers = { FRC,  FLC,  RR,  RL,  FC,  LFE,  FR,  FL } },
		.channels = 8,
		.spk_mask = FRC | FLC | RR | RL | FC | LFE | FR | FL,
		.spk_na_mask = 0x0,
	},

I would prefer the first one to save lines, but i can implement the
second one (or another one if you have an alternative to propose).

Regards
Arnaud
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel