linux-kernel - Re: [PATCH v2 05/13] gpu: ipu-v3: Add IDMA channel linking support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <11242b1e-b709-d60f-213b-be78d4a596f9@mentor.com>
Date:	Thu, 28 Jul 2016 16:40:42 -0700
From:	Steve Longerbeam <steve_longerbeam@...tor.com>
To:	Philipp Zabel <p.zabel@...gutronix.de>,
	Steve Longerbeam <slongerbeam@...il.com>
CC:	<plagnioj@...osoft.com>, <tomi.valkeinen@...com>,
	<dri-devel@...ts.freedesktop.org>, <linux-fbdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 05/13] gpu: ipu-v3: Add IDMA channel linking support



On 07/26/2016 03:06 AM, Philipp Zabel wrote:
> Am Dienstag, den 19.07.2016, 18:11 -0700 schrieb Steve Longerbeam:
>> Adds functions to link and unlink IDMAC source channels to sink
>> channels.
>>
>> So far the following links are supported:
>>
>> IPUV3_CHANNEL_IC_PRP_ENC_MEM -> IPUV3_CHANNEL_MEM_ROT_ENC
>> PUV3_CHANNEL_IC_PRP_VF_MEM   -> IPUV3_CHANNEL_MEM_ROT_VF
>> IPUV3_CHANNEL_IC_PP_MEM      -> IPUV3_CHANNEL_MEM_ROT_PP
>>
>> More links can be added to the idmac_link_info[] array.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@...tor.com>
> This patch looks good to me, but the Frame Synchronisation Unit supports
> also linking the internal channels, not only the IDMAC channels.
>
> [...]
>> +++ b/drivers/gpu/ipu-v3/ipu-common.c
> [...]
>> +static const struct idmac_link_info idmac_link_info[] = {
>> +	{
>> +		.src  = { IPUV3_CHANNEL_IC_PRP_ENC_MEM, IPU_FS_PROC_FLOW1,
>> +			  0, 4, 7 },
>> +		.sink = { IPUV3_CHANNEL_MEM_ROT_ENC, IPU_FS_PROC_FLOW2,
>> +			  0, 4, 1 },
>> +	}, {
>> +		.src =  { IPUV3_CHANNEL_IC_PRP_VF_MEM, IPU_FS_PROC_FLOW1,
>> +			  8, 4, 8 },
>> +		.sink = { IPUV3_CHANNEL_MEM_ROT_VF, IPU_FS_PROC_FLOW2,
>> +			  4, 4, 1 },
>> +	}, {
>> +		.src =  { IPUV3_CHANNEL_IC_PP_MEM, IPU_FS_PROC_FLOW1,
>> +			  16, 4, 5 },
>> +		.sink = { IPUV3_CHANNEL_MEM_ROT_PP, IPU_FS_PROC_FLOW2,
>> +			  12, 4, 3 },
>> +	},
>> +};
> How about adding new (internal) channel numbers for the CSI->VDI link
> and having something like:
>
> 	{
> 		.src =  { IPUV3_CHANNEL_CSI_DIRECT, IPU_FS_PROC_FLOW1,
> 			  FS_VDI_SRC_SEL_OFFSET, 2, 1 },
> 		.sink = { IPUV3_CHANNEL_VDI_CUR, 0, 0, 0 },
> 	},
>
> instead of ipu_set_vdi_src_mux? Then in addition to
>
> [...]
>> +int ipu_idmac_link(struct ipuv3_channel *src, struct ipuv3_channel *sink)
> We could have a lower level
>
> ipu_fsu_link(int channel1, int channel2)
>
> which could be called like
>
> ipu_fsu_link(IPUV3_CHANNEL_CSI_DIRECT, IPUV3_CHANNEL_VDI_CUR);
>
> Come to think of it, could we replace shift,bits,sel with mask,value and
> add #defines for the FSU bit fields and values? I'm thinking:
>
> /* FS_PROC_FLOW1 */
> #define FS_PRPENC_ROT_SRC_SEL_MASK	(0xf << 0)
> #define FS_PRPENC_ROT_SRC_SEL_ENC		(0x7 << 0)
> #define FS_PRPVF_ROT_SRC_SEL_MASK	(0xf << 8)
> #define FS_PRPVF_ROT_SRC_SEL_VF			(0x8 << 8)
> #define FS_PP_SRC_SEL_MASK		(0xf << 12)
> #define FS_PP_ROT_SRC_SEL_MASK		(0xf << 16)
> #define FS_PP_ROT_SRC_SEL_PP			(0x5 << 16)
> #define FS_VDI1_SRC_SEL_MASK		(0x3 << 20)
> #define FS_VDI3_SRC_SEL_MASK		(0x3 << 20)
> #define FS_PRP_SRC_SEL_MASK		(0xf << 24)
> #define FS_VDI_SRC_SEL_MASK		(0x3 << 28)
>
> /* FS_PROC_FLOW2 */
> #define FS_PRP_ENC_DEST_SEL_MASK	(0xf << 0)
> #define FS_PRP_ENC_DEST_SEL_IRT_ENC		(0x1 << 0)
> #define FS_PRPVF_DEST_SEL_MASK		(0xf << 4)
> #define FS_PRPVF_DEST_SEL_IRT_VF		(0x1 << 4)
> #define FS_PRPVF_ROT_DEST_SEL_MASK	(0xf << 8)
> #define FS_PP_DEST_SEL_MASK		(0xf << 12)
> #define FS_PP_DEST_SEL_IRT_PP			(0x3 << 12)
> #define FS_PP_ROT_DEST_SEL_MASK		(0xf << 16)
> #define FS_PRPENC_ROT_DEST_SEL_MASK	(0xf << 20)
> #define FS_PRP_DEST_SEL_MASK		(0xf << 24)
>
> struct idmac_link_reg_info {
> 	int chno;
> 	u32 reg;
> 	u32 mask;
> 	u32 val;
> };
>
> static const struct idmac_link_info idmac_link_info[] = {
> 	{
> 		.src  = { IPUV3_CHANNEL_IC_PRP_ENC_MEM, IPU_FS_PROC_FLOW1,
> 			  FS_PRPENC_ROT_SRC_SEL_MASK, FS_PRPENC_ROT_SRC_SEL_ENC },
> 		.sink = { IPUV3_CHANNEL_MEM_ROT_ENC, IPU_FS_PROC_FLOW2,
> 			  FS_PRP_ENC_DEST_SEL_MASK, FS_PRP_ENC_DEST_SEL_IRT_ENC },
> 	}, {
>                 .src =  { IPUV3_CHANNEL_IC_PRP_VF_MEM, IPU_FS_PROC_FLOW1,
>                           FS_PRPVF_ROT_SRC_SEL_MASK, FS_PRPVF_ROT_SRC_SEL_VF },
>                 .sink = { IPUV3_CHANNEL_MEM_ROT_VF, IPU_FS_PROC_FLOW2,
>                           FS_PRPVF_DEST_SEL_MASK, FS_PRPVF_DEST_SEL_IRT_VF },
> 	}, {
>                 .src =  { IPUV3_CHANNEL_IC_PP_MEM, IPU_FS_PROC_FLOW1,
>                           FS_PP_ROT_SRC_SEL_MASK, FS_PP_ROT_SRC_SEL_PP },
>                 .sink = { IPUV3_CHANNEL_MEM_ROT_PP, IPU_FS_PROC_FLOW2,
>                           FS_PP_DEST_SEL_MASK, FS_PP_DEST_SEL_IRT_PP },
> 	},
> };

Hi Philipp,

Great ideas. I'll work on this for next version, or shall I let you? 
Should we create an
ipu-fsu.c ?

Steve