Message-ID: <4d670a5c-b680-ba76-a640-88b3d945c0b0@amd.com>
Date:   Fri, 3 Mar 2023 15:35:58 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] drm/amd/display: Simplify same effect if/else blocks
Content-Language: en-US
From:   Harry Wentland <harry.wentland@amd.com>
To:     Deepak R Varma <drv@mailo.com>, Joe Perches <joe@perches.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        Aurabindo Pillai <aurabindo.pillai@amd.com>
Cc:     Leo Li <sunpeng.li@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        Saurabh Singh Sengar <ssengar@microsoft.com>,
        Praveen Kumar <kumarpraveen@linux.microsoft.com>
References: <Y8POxreeC3EvOXhC@ubun2204.myguest.virtualbox.org>
 <33ecbe8971bb9c90d72c67d43ca740abac160908.camel@perches.com>
 <Y82GHzOUHgEqTUq7@ubun2204.myguest.virtualbox.org>
 <Y/+z22xM7NNjX8VP@ubun2204.myguest.virtualbox.org>
 <f15b10a8-11f6-6c1f-c94e-71796aad9155@amd.com>
In-Reply-To: <f15b10a8-11f6-6c1f-c94e-71796aad9155@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QU5OMldnMm0rSWNLM3JVbGx6cDArVW9wYkxsUTRlcnVySnQ3VkxmcEg1RWdW?=
 =?utf-8?B?cXpRZERQcFQ2Z1gvekZoSVpjcDVGOXpKQlN0TG85VndxTU1NRXgycCtTdDhH?=
 =?utf-8?B?Y0paN1dUdTZxK2FnYm9IbjlSTlpiVXYyZjFBdHVyM3Qxb2xoTVozbGd1U1g0?=
 =?utf-8?B?SERRR01RYmRzakhXWmxVRkQvZ1JEeDR5S3hiY0dGdXRYSGN5UktJTlZXRXUx?=
 =?utf-8?B?aFZBWCtPbzVISzdPWERtSGl4Q0lOd3Mya0Q5TFM5dUo1SGlyV3E5TDBZZ2ZG?=
 =?utf-8?B?eFVRUUgrcDV6YnY5dmRGMHFIMGVidENYQ2FiTEkvQTJRa2V5eWY1Q2Fpc21T?=
 =?utf-8?B?YXFTRFlWZ1FuZDZiamdkY3dZc2t5eFhXZ1Mwbk1MSXhpTm1NT0E2U1pXbG1X?=
 =?utf-8?B?bUZaTkk4VWZFaVZNTzdhKzZRU3AzS1FOUFRSOHVpalBmRWx5eFJZTm1UTVBs?=
 =?utf-8?B?d3h5ZzltU0FlNC9GQkVoUjZvN3hWN0lTbFJ3ek8ydEhUYWNtbjFVSkRGMW5K?=
 =?utf-8?B?eW9WN2tuS3RxRVhRLzJjL1dFdTJZWlRWK3d1NlNmVCt1WE5YT2k3NnRxOG1K?=
 =?utf-8?B?dTRzamFrc2J1UG9rVlI0QzhxTnlJVDdkWThZWlI5MDFYN0lKWlpRYU94bnM3?=
 =?utf-8?B?RWtwSkxIMForWUtwWHRzUi9RM3B3YWpML2dCbFA3TEtJQkkxdlVoTnpWamh3?=
 =?utf-8?B?bWJVMGNrN3h6c0YrRzduZGprMUYxeFNXK0p3MHh0cHFYVHN6eXBvZ0RkS29M?=
 =?utf-8?B?NUo5QUJmeVBSVTJsZ2VoZVUzNU1ad1daREQwSy80OENCczJ0c25pYW0yQis2?=
 =?utf-8?B?eWMyQ01HUTNjb3FyWCs1ZjFFTHBYazJ5Y3cycVFGQUd4eDQwcFE2SGJYczRq?=
 =?utf-8?B?NVB3S0xTcjJIQW1XZjVFME5jZGF2S3NzUWlkYXVhVTh0OUd4a0tFRUJpd0dF?=
 =?utf-8?B?REt0WG90S05LaTNGTkUxQ2hITDEwNXJ3dU5tejROUUxpWXRnNHlzOC9obUpZ?=
 =?utf-8?B?S3lMR2FzN0YwekJHTng3eTI0bTY2VVRwOE1STHg1N05WbWxqcWxjTkRwc0xn?=
 =?utf-8?B?aldZWXhJV28xTExXc3JVM3pGcWd1QmVudVJ1cnlhT3NoVG1zVi9qUmNqc0ZH?=
 =?utf-8?B?M20xRVhwNnVETGZMQ21Ua3pMcDBvc3hVZSt6YXl3RGFlVXBxRC9MeXA2K2Fo?=
 =?utf-8?B?STI1a2ZCNTVnRi9oR00zeW1USEUwZko5Zy85cjYwNFphdU5GNGJSb1FmNm5R?=
 =?utf-8?B?cWNmOXAvLzZnejl5MjN0TXJobExpREFVTGpaeXVzRW52MlljcUlJQkREdjhD?=
 =?utf-8?B?NmRCVXZuWXZKRlZIQVQxclpEbnlhZnNUTHFHMzNGMWFMVFlCT3VuQkJVaHlT?=
 =?utf-8?B?ajVEZ0pOcTFaa0xkcmU2UkF5WER1NG9XaURJTjlzbWViSlRQc0NhZkxNOTdI?=
 =?utf-8?B?ZUpReEh2UjZPdDJYMjkrcFp2K1F4M3hNZXlvcElVWEw4cDJ6dngwaE1KU1gy?=
 =?utf-8?B?azRuLzA0RDJuMWZvd29lVzZSR3lleUVtb0hVRkVZWUd5YWNvalFFTjNRL2xS?=
 =?utf-8?B?L3RjeTVva1ZSV2JDdjlLb3NGZ0c0anZINi9XM1FQWTQxU3ZXbVBYKzFNOEha?=
 =?utf-8?B?TTBaK3lhcTRzcURZMU5ZbXZwUGF6TFBabGJCMHBWQUo4SWZNbVdBTmZRa1Zn?=
 =?utf-8?B?SFBOMTFWd25hMWRvRFphYkxmblVuMmxRckhQdm1udmZRYzlIOFFvSWJ6U21S?=
 =?utf-8?B?RThwL1B4TGdWMEZDRzZEL2RmQVhwYlN5SGJRTjFDOXpJc01jODBoVTMrUjN2?=
 =?utf-8?B?OVMvOXdwYUV1ek9RMk05SjFnWkJieGVRQWNvNlZ2NXhQQnMrTldybXF1MXV1?=
 =?utf-8?B?TVRVVHd1WlVTSDY0K2gyS1IyejFSUjRZdHpJNzJEUEpyQ01kQ2JmckJEUmtx?=
 =?utf-8?B?bDJzbmp4WlQ4TWNTamdaNVF0dmxTY1BvNGx6bW96Uk1VZVRmMEpXbjlCQWVO?=
 =?utf-8?B?aFF1SVFqWk9lRmVwTWlaWkdJcDRRUmluZXc4ZitZeW94WCtTMXAzeGRJT2xx?=
 =?utf-8?B?OXJLM1NFbWdKUEluYjQ0RWtMcnBaNDFJZDd2R3prbCtUbmRpajArYkVRWHJ5?=
 =?utf-8?Q?FHLg+JPVSa4A0V4qbLnenBqsl?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 796a42f5-67e9-4553-8686-08db1c26e74a
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2023 20:36:02.0133
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Q7EMzeqk/IL5JG40NY6G0WJnuMWMbu4c5Sp/MxXSBAACAoALG/YTs05KSLF231G1iTeUYEkWgEprFYq+dPZ08Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5844
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/2/23 11:37, Harry Wentland wrote:
> 
> 
> On 3/1/23 15:21, Deepak R Varma wrote:
>> On Mon, Jan 23, 2023 at 12:23:19AM +0530, Deepak R Varma wrote:
>>> On Sun, Jan 15, 2023 at 12:52:10PM -0800, Joe Perches wrote:
>>>> On Sun, 2023-01-15 at 15:30 +0530, Deepak R Varma wrote:
>>>>> The if / else block code has same effect irrespective of the logical
>>>>> evaluation.  Hence, simply the implementation by removing the unnecessary
>>>>> conditional evaluation. While at it, also fix the long line checkpatch
>>>>> complaint. Issue identified using cond_no_effect.cocci Coccinelle
>>>>> semantic patch script.
>>>>>
>>>>> Signed-off-by: Deepak R Varma <drv@mailo.com>
>>>>> ---
>>>>> Please note: The proposed change is compile tested only. If there are any
>>>>> inbuilt test cases that I should run for further verification, I will appreciate
>>>>> guidance about it. Thank you.
>>>>
>>>> Preface: I do not know the code.
>>>>
>>>> Perhaps Rodrigo Siqueira made a copy/paste error submitting the code for
>>>> commit 9114b55fabae ("drm/amd/display: Fix SubVP control flow in the MPO context")
>>>> as the code prior to this change is identical.
>>>>
>>>> Perhaps one of the false uses should be true or dependent on the
>>>> interdependent_update_lock state.
>>>
>>> Thank you Joe for the recommendation.
>>>
>>> Hi Rodrigo,
>>> Can you review and comment on if and what is wrong with your commit?
>>
>> Hello Rodrigo, Alex,
>> Could you please suggest what would be the necessary fix for this typo error?
>>
> 
> It's not quite a "typo" error. When I look at this code in our internal repo I see
> a couple missing lock calls here that differ between the two cases. I don't know why
> this was never ported over and am surprised it doesn't lead to issues.
> 
> I would prefer we keep the code as-is for now until this gets sorted.
> 

Actually I was wrong. Too many similar-looking snippets in this
function made me look at the wrong thing. This change is fine and
Reviewed-by: Harry Wentland <harry.wentland@amd.com.

Harry

> Harry
> 
>> Thank you,
>> Deepak.
>>
>>>
>>> Thank you,
>>> ./drv
>>>
>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
>>>> []
>>>>> @@ -3470,14 +3470,9 @@ static void commit_planes_for_stream(struct dc *dc,
>>>>>  		/* Since phantom pipe programming is moved to post_unlock_program_front_end,
>>>>>  		 * move the SubVP lock to after the phantom pipes have been setup
>>>>>  		 */
>>>>> -		if (should_lock_all_pipes && dc->hwss.interdependent_update_lock) {
>>>>> -			if (dc->hwss.subvp_pipe_control_lock)
>>>>> -				dc->hwss.subvp_pipe_control_lock(dc, context, false, should_lock_all_pipes, NULL, subvp_prev_use);
>>>>> -		} else {
>>>>> -			if (dc->hwss.subvp_pipe_control_lock)
>>>>> -				dc->hwss.subvp_pipe_control_lock(dc, context, false, should_lock_all_pipes, NULL, subvp_prev_use);
>>>>> -		}
>>>>> -
>>>>
>>>> Perhaps something like:
>>>>
>>>> 		if (dc->hwss.subvp_pipe_control_lock)
>>>> 			dc->hwss.subvp_pipe_control_lock(dc, context,
>>>> 							 should_lock_all_pipes &&
>>>> 							 dc->hwss.interdependent_update_lock,
>>>> 							 should_lock_all_pipes, NULL, subvp_prev_use);
>>>>
>>>>> +		if (dc->hwss.subvp_pipe_control_lock)
>>>>> +			dc->hwss.subvp_pipe_control_lock(dc, context, false, should_lock_all_pipes,
>>>>> +							 NULL, subvp_prev_use);
>>>>>  		return;
>>>>>  	}
>>>>>  
>>>>
>>>
>>>
>>
>>
>