Received-SPF: pass (google.com: domain of linux-kernel+bounces-163573-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <84aa4bfb-d3fa-4721-98a2-dad0cc753434@oss.nxp.com>
Date: Tue, 30 Apr 2024 16:55:06 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 3/3] media: amphion: Remove lock in s_ctrl callback
To: Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
 Ming Qian <ming.qian@nxp.com>, mchehab@kernel.org, hverkuil-cisco@xs4all.nl
Cc: shawnguo@kernel.org, robh+dt@kernel.org, s.hauer@pengutronix.de,
 kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
 xiahong.bao@nxp.com, eagle.zhou@nxp.com, tao.jiang_2@nxp.com,
 imx@lists.linux.dev, linux-media@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
References: <20240425065011.105915-1-ming.qian@nxp.com>
 <20240425065011.105915-3-ming.qian@nxp.com>
 <8c39b3c3-8146-4418-8835-6dbfe38a85ec@collabora.com>
 <be54f273-7bba-4db0-bc52-5ddbb3982d84@oss.nxp.com>
 <0c0e2d02-b719-4f8b-afca-c839bb77f287@collabora.com>
Content-Language: en-US
From: ming qian <ming.qian@oss.nxp.com>
In-Reply-To: <0c0e2d02-b719-4f8b-afca-c839bb77f287@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: PAXPR04MB8254:EE_|AS8PR04MB7942:EE_
X-MS-Office365-Filtering-Correlation-Id: c23d68e2-51bd-442f-9773-08dc68f25d9a
X-MS-Exchange-SharedMailbox-RoutingAgent-Processed: True
X-MS-Exchange-SenderADCheck: 1
X-MS-Exchange-AntiSpam-Relay: 0
X-Microsoft-Antispam: BCL:0;ARA:13230031|366007|376005|7416005|1800799015;
X-Microsoft-Antispam-Message-Info:
	=?utf-8?B?YSt1bGJxbXdpcTdwbEZzbTJHL2xGWGdmeFo2MEZHZWNHb2pqZTlyUUphYnBn?=
 =?utf-8?B?cDVPVnVGYU5GNENvZ3NKR0hyNzlIaXBMVGtFMytqMGh5Z2pkWlNSakE5V21H?=
 =?utf-8?B?dGZTRk1iM3hKZGkzS2w4UXIyS1o0MldWb2xMRFpBWkI1RnhjNGlYb1h3UXdE?=
 =?utf-8?B?MFVmUmVHNzZpY2VsRGplSUxRUVNINFBHaUppYjF5S2p5VVBBWlAyenNsQTZQ?=
 =?utf-8?B?a012azNJakZLUXJ0T2xhTmNWbXlvRHB5UmpaU2RYczFta0oyVEk4OXVjU0NH?=
 =?utf-8?B?ZnhDaGMxR3lPVjRybGM0ZGRXdVpTc3BUZFh6ZWdMKzFvcVQ5S3c4MXZNTTJY?=
 =?utf-8?B?NU40RmdsK2VxVjNWNFBlbTJzOElqRlJSOVI5OXlhWHk1ZkFpTlArMUFyaWJR?=
 =?utf-8?B?UVA4L1dlWVROZmUxVDc4dlNMYXRERGpDamEvWnNESDhrOGlBQ0dQYVorTVVq?=
 =?utf-8?B?dWNFLzA5N2FQYURnbXZUaVNTeTFsWWtlNUZ3U0tHcEJuSFFjWU9ZV0FURWF6?=
 =?utf-8?B?bE9lN3kwVVRnWG5aZDU4QVVBdnVWOVMwdjErUVZjQzlHZ2RDQWZIMmQ4WTAy?=
 =?utf-8?B?RGpHbGxzWnlHQWVZOGZBWHIzb3psSDFkdGxVYVdMVGp6Mm1DOXduQ1pqNTBP?=
 =?utf-8?B?dVBDbEtyam5oR1dWOCtKWjZidjhFNkRZbE43RjVXYWgrOXFiSWVqMWZZcG1X?=
 =?utf-8?B?QW9ZV1lWWnhFYythU0psdnRpcFZjNXVqRTBRd3B6QTNSUUFuQTI0T1BkdnZa?=
 =?utf-8?B?UFJwN2QzMVRONXRMNkFjTGZUK2V4ckdnMTl6UitSL0ZaMGtNMzczejBGbHMy?=
 =?utf-8?B?UFpNVUZkbXNkdVVLWVVHQVBHZk1MT0U0S1gvOXg4NyttdVBGeW1HRTU0dTdV?=
 =?utf-8?B?RkNERCtMNmR2MDFzY09BRnl0R1R2REdlaGV3bnQ4WURobkkrRHVpNThZMGpU?=
 =?utf-8?B?eXZ4cGNvTTZhNitwb2xESHlyK1QvUVY4UkpxUHgrYzlTcy9LNmd3aEU0TDBh?=
 =?utf-8?B?NVFMZHFRZVJRbzBsZ1gzWTc5SlNxQ3NwRDE4MlFITmxwMDZhd2RYZWtXN0tt?=
 =?utf-8?B?TWNMejRNMzdINnE1T1cyRndpaXBpLzlrOWdsZHJTZkUrNExmUzNXRmRBcVRa?=
 =?utf-8?B?T3I0SHRhZG9PanprSkV2R0w0cnVab0lSaUJvSVBRR09ySHFFTEVWQWhVVDVp?=
 =?utf-8?B?SnM3QU1JUDlBZmk0aWtxckJaeUY5WlZwWGZUVmN3dEtUdG1hUzgyMzBONWdL?=
 =?utf-8?B?RHBScnhCc29Hc2pmUEhRak13QTNsb0xZZVEyTkoweEs5bjNJdUFyTkdMcW1q?=
 =?utf-8?B?enQ0U3VMR0JBMTNKUHhUVnM1R29iNEVzSDNnYzNib3U2NkNWL1BEMnJFWnFE?=
 =?utf-8?B?c1lVTjJCZXNKLzUybVNPWWFnYzVLNysxc1dic2tpeDRMWk1SazFvL2QwcmRZ?=
 =?utf-8?B?MUJ4TDhPVjBYS2ExVkp0aStvYmptbHl2SE92V24xK3dpM21pdjJaSFliSXM4?=
 =?utf-8?B?a2ZRQ24rVmZPSUx4elNjeVhxbVhQVmZwbkViUzNjczlOTzZQMnNMdEU0cmlz?=
 =?utf-8?B?MzhTcStOR0xRSVNlOExCN2xUYW1iWHBpcmJWTGpwZjZ5NHg5SnZPZUFrQWJq?=
 =?utf-8?B?K3ZxdkFRMjdxdUpCNTBRWC81MUVKOEdsUkVHQk1GMFlJL2pGVEhYd0UzREpa?=
 =?utf-8?B?YUpDeDIybDB4MnJ3cldXWkpYeENpTytqUjZQUEFRZ1RuT2FlY2xwQkl3PT0=?=
X-Forefront-Antispam-Report:
	CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:PAXPR04MB8254.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(366007)(376005)(7416005)(1800799015);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?bWJKYVdKMlExU3dRSklaTFRKSCtnbXhKejArei9KWDA1bFdVUVlwODFDMDYw?=
 =?utf-8?B?YVFoaXlkNzVteUx0VVJmcmdsNUxUOGdyZ1h3UTNvdnVQeDRwTWx2Rng1dTNm?=
 =?utf-8?B?aHdDZ2VmMFZuZmZpMHlFa1lvY0F4bnp5djNTY1o3Z1MxWExQOExFQWIreVBW?=
 =?utf-8?B?RE95ZTJYa3VVd1IzZ042NWptQ0Y0aEVhcitESCt0WEhaU2NZMkJHVy8ybnl3?=
 =?utf-8?B?cDBoVUlOVjU0dG9xdTAyV09KM0NJMktTQnFVL2Z5bGU4NHlaSFBvVjFPbm1L?=
 =?utf-8?B?VHF0dFB1VUN0ekU5aThrbHF2ci9QM0JCQWdxMk4rd0RjLzIzTVVLS2thU1hQ?=
 =?utf-8?B?U0V6VTNNa3grYXpqZldlTVQ4ZkFjbmt2Zllla2NYQjg2cSt1V01tY3Y1ckMr?=
 =?utf-8?B?VkJYaEVVbytEKytoSk9iMXB4blhHbXpqa0Yyc2N5dEVtejV0OW1rMlJkTDZK?=
 =?utf-8?B?RU90eTZXZHdCd1QzWjIvZW1jMFJrNE1WZzNhd054eGQ0UlFHK2ZIMnJ3OWZK?=
 =?utf-8?B?ZWFmZHE2T2l1TnFuOEg3NGdpZm53aVNMVUZpTVcrQlVRcEpIOXhOTG9xSnR1?=
 =?utf-8?B?bXlMUVV2U1dYRUJQWVJXWDNoN2dQajNCdHRPVmQ4WHFaUmhQZ0tJQzdkbDAx?=
 =?utf-8?B?cUZGNkVLZk9jaGM0TVVMand4bUt0dnp4K3VoQk1FUUJqcGRYb2JDeFNYeVp4?=
 =?utf-8?B?eDBVM1c2U2RvRFM0c2pyUjdocERHWWhsWFRtOVhHNzJnS1BsbTkxeG45Mm1o?=
 =?utf-8?B?Zmo0bkhkWHhMSUJDZVNWZVNib0h5SFo5MnFRWkpIMVdHMmdsRUNxeEdnd1ZF?=
 =?utf-8?B?QkExS1dXc01ha3FScjlQREZjTmpMVFZIc2pPTGVkVEN3K0xqcTJjQ3JqZEc0?=
 =?utf-8?B?Z1ZEUWRlUjJOQU5VQ2NUQm15ZUZOZnFlM3o2TWYvdzg0cDNGRDg1M3U0VzBF?=
 =?utf-8?B?dUFNT0QrQ29DM2liL2pZYTBudCtGMDQwVU5qZFpHdFZUWFhRN3hGUHN4bVlr?=
 =?utf-8?B?TlNaMTdaZjllQzBxYmJjWlAveDJYWFFtK3JjTHUwNTVRL0oyNTRSUWJMRmdD?=
 =?utf-8?B?WDl4TW1YTW9VaXR0dnF5Q05xSWR3K1VQbDk0cEZvR0Nvd3RBeWtIOWNpbDEy?=
 =?utf-8?B?Wm0xcjBsOW5OeERZdGYvVWoxb0pBS2U2K3d6MlcybWJsbzQ2cTFGU3lEdXo5?=
 =?utf-8?B?dHZmSThpdE5ORWsvTzdsTVVqN1cvRUdjSlljVlAxSW1Bd3dINHRpeUJwOHpP?=
 =?utf-8?B?YkwrZG9xS1ZoK0FOZ2NobEdlVGkycEZHNnVPREhsZVQ3UGpJRFRGdzN3aW1L?=
 =?utf-8?B?ektDU0FBci93bW5wN1R4MzhmSU9nbnhiVlQ5Si81WFRJcUxnNEh4MW9MMTUv?=
 =?utf-8?B?dndJOG1acWZtUmlpTkhWN2VyVlpYYXIrLzk4SzNPODczQnB1MDNJYjQ4RXhC?=
 =?utf-8?B?Q05jMXFVNkIybjFnUEtMYVN3MjBRWkVLVEZqSDBwYkF2bC8wK2FqdHVNb0tx?=
 =?utf-8?B?NWQ5ZG03OTdQdUdtZEtkRTUzODUwenVVMDA4ZWVXZ1dDanhxUTYxL2hTOGcy?=
 =?utf-8?B?YU9jMStlTmtJeDZaYm1TVjRoVjN1Mkc2SkFjemZvSWhrK0c3NE5pMU5RNFNH?=
 =?utf-8?B?OHNxQ0NoQmE4OEhqQ3JUZldKdEkwbTQzeXFPMVI2NjIxWUw0ckZaaTVTbXd0?=
 =?utf-8?B?L3UzU1M2M0w2NmxYNnpmNklGOW5yRlNhZ1BOQW0xdzBvdm9IYlp3MWdjTDZO?=
 =?utf-8?B?U0lDcVA5MGpTck9IaFlMU3kvdTRXbmN5WnNrS2N6U0hvUEthcDhzQ3M5NndC?=
 =?utf-8?B?Zm0zK092Q0FKczFxWG9DS2FNUVdFNTZaOEN3NzduVUNFV1ZvdXQyUFdRUUJs?=
 =?utf-8?B?K2VtWHpBczNERE9yZlNqRi9CelhCdHlpMENoZUxEeGZzWURCV01Kc2NDN0cy?=
 =?utf-8?B?RHhMTHIwSnBZczZlR2RDNjV3blJaV3dYK1k5RkFtdFUrL0swZkRkOXBQeVVF?=
 =?utf-8?B?aWxQNlRTMDFCUEdnL3c0WUVPWkQ2NjFXLytodGg0UzI1K05PUHNsZUp4WGlt?=
 =?utf-8?B?eTJhT2J0aTA2U3p1cHZhd3h3UEErVjFtOHN3ZkVBLys2NkkzbEQ5Q0lMaDFL?=
 =?utf-8?Q?qmdc2C61HVoiZWWQAhTqJkmcF?=
X-OriginatorOrg: oss.nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c23d68e2-51bd-442f-9773-08dc68f25d9a
X-MS-Exchange-CrossTenant-AuthSource: PAXPR04MB8254.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2024 08:48:54.4891
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ZEhh4blZLrSvUT5kBISuIwfgi5n2ZXtK62bOvjkE5+hmFGYNpzNQvWwpmlvqZfMo93YHIZ872gXkbCTmE/ncDA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB7942


Hi Andrzej,

> Hi Ming,
> 
> W dniu 30.04.2024 o 04:32, ming qian pisze:
>> Hi Andrzej,
>>
>>> Hi Ming Qian,
>>>
>>> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>>>> There is no need to add lock in s_ctrl callback, it has been
>>>> synchronized by the ctrl_handler's lock, otherwise it may led to
>>>> deadlock if driver call v4l2_ctrl_s_ctrl().
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>>> ---
>>>>   drivers/media/platform/amphion/vdec.c | 2 --
>>>>   drivers/media/platform/amphion/venc.c | 2 --
>>>>   2 files changed, 4 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/amphion/vdec.c 
>>>> b/drivers/media/platform/amphion/vdec.c
>>>> index a57f9f4f3b87..6a38a0fa0e2d 100644
>>>> --- a/drivers/media/platform/amphion/vdec.c
>>>> +++ b/drivers/media/platform/amphion/vdec.c
>>>> @@ -195,7 +195,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>       struct vdec_t *vdec = inst->priv;
>>>>       int ret = 0;
>>>> -    vpu_inst_lock(inst);
>>>
>>> I assume that PATCH v2 2/3 might cause the said deadlock to happen?
>>> If so, maybe it would make more sense to make the current patch preceed
>>>   PATCH v2 2/3? Otherwise the kernel at PATCH v2 2/3 introduces a 
>>> potential
>>> deadlock.
>>>
>>> Regards,
>>>
>>> Andrzej
>>>
>>
>> I actually discovered this problem when I was preparing the v2 2/3 patch.
>>
>> But in the v2 2/3 patch, it tried to add a read-only ctrl, then I just
>> unset the s_ctrl callback for the new added ctrl, the potential deadlock
>> is caused by call the s_ctrl back in a locked environment, so after unset
>> the s_ctrl callback, the 2/3 patch won't trigger the deadlock even if
>> this patch is missing.
>>
>> In order to avoid encountering similar problems in the future, that
>> driver may set or get some ctrl, I added this patch.
> 
> Do I understand you correctly that patch 2/3 is written in such a way that
> it does not introduce a deadlock, and you add patch 3/3 only to prevent 
> future
> problems? If so, it seems to me that patch 3/3 could/should be separate 
> from
> this series, as it does not quite match "Add average qp control".
> 
> Regards,
> 
> Andrzej
> 

You're right, when I prepare the 2/3 patch, I add the new ctrl just like
the previous ctrl, but I got a deadlock. then I tried to fix a deadlock
in 2/3 patch first, and make this patch again to avoid similar errors.

I put this patch to this series, as I think this better explains why I
made this change.

Regards,
Ming

>>
>> Best regards,
>> Ming
>>
>>>>       switch (ctrl->id) {
>>>>       case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
>>>>           vdec->params.display_delay_enable = ctrl->val;
>>>> @@ -207,7 +206,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>           ret = -EINVAL;
>>>>           break;
>>>>       }
>>>> -    vpu_inst_unlock(inst);
>>>>       return ret;
>>>>   }
>>>> diff --git a/drivers/media/platform/amphion/venc.c 
>>>> b/drivers/media/platform/amphion/venc.c
>>>> index cdfaba9d107b..351b4edc8742 100644
>>>> --- a/drivers/media/platform/amphion/venc.c
>>>> +++ b/drivers/media/platform/amphion/venc.c
>>>> @@ -518,7 +518,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>       struct venc_t *venc = inst->priv;
>>>>       int ret = 0;
>>>> -    vpu_inst_lock(inst);
>>>>       switch (ctrl->id) {
>>>>       case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>>>>           venc->params.profile = ctrl->val;
>>>> @@ -579,7 +578,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>           ret = -EINVAL;
>>>>           break;
>>>>       }
>>>> -    vpu_inst_unlock(inst);
>>>>       return ret;
>>>>   }
>>>
>