From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A8825C02185 for ; Fri, 17 Jan 2025 13:07:58 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2B38710EAE6; Fri, 17 Jan 2025 13:07:58 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (1024-bit key; unprotected) header.d=linux.dev header.i=@linux.dev header.b="pArEjGXs"; dkim-atps=neutral Received: from out-189.mta1.migadu.com (out-189.mta1.migadu.com [95.215.58.189]) by gabe.freedesktop.org (Postfix) with ESMTPS id 52D1A10EAF1 for ; Fri, 17 Jan 2025 13:07:55 +0000 (UTC) Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1737119272; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=vk+L5/wjKvymhB96CaUuZqmjix8B8veXegV3r8yTl50=; b=pArEjGXs1VyXklZzkySQMIBcjU0P2r6bmEo+gY0UK3jRfi0ol3d7NaTEm1DoEWEEQ1xRw6 xrXDq5/iYKGxCKa7MQ+8rbjOP0jEpTLzpgAFL4fex0qO+fYPvBxjVPhyVpOE/zGThkQeOV 699uNkpp4WKsn7oc5REDjRzI9cwNRYk= Date: Fri, 17 Jan 2025 18:37:00 +0530 MIME-Version: 1.0 X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Aradhya Bhatia Subject: Re: [PATCH v7 11/12] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable To: Dmitry Baryshkov Cc: Tomi Valkeinen , Laurent Pinchart , Andrzej Hajda , Neil Armstrong , Robert Foss , Jonas Karlman , Jernej Skrabec , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Nishanth Menon , Vignesh Raghavendra , Devarsh Thakkar , Praneeth Bajjuri , Udit Kumar , Jayesh Choudhary , DRI Development List , Linux Kernel List References: <20250114055626.18816-1-aradhya.bhatia@linux.dev> <20250114055626.18816-12-aradhya.bhatia@linux.dev> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Dmitry, On 14/01/25 16:54, Dmitry Baryshkov wrote: > On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote: >> Move the bridge pre_enable call before crtc enable, and the bridge >> post_disable call after the crtc disable. >> >> The sequence of enable after this patch will look like: >> >> bridge[n]_pre_enable >> ... >> bridge[1]_pre_enable >> >> crtc_enable >> encoder_enable >> >> bridge[1]_enable >> ... >> bridge[n]_enable >> >> And, the disable sequence for the display pipeline will look like: >> >> bridge[n]_disable >> ... >> bridge[1]_disable >> >> encoder_disable >> crtc_disable >> >> bridge[1]_post_disable >> ... >> bridge[n]_post_disable >> >> The definition of bridge pre_enable hook says that, >> "The display pipe (i.e. clocks and timing signals) feeding this bridge >> will not yet be running when this callback is called". >> >> Since CRTC is also a source feeding the bridge, it should not be enabled >> before the bridges in the pipeline are pre_enabled. Fix that by >> re-ordering the sequence of bridge pre_enable and bridge post_disable. > > The patch contains both refactoring of the corresponding functions and > changing of the order. Please split it into two separate commits. > I assume that you already understand this, so this is just for the record - There is no trivial way to do this. Initially, this is what I had in my mind - about what the split patches would look like. #1 * refactoring of entire loops into separate functions. * Separate out the pre_enable and enable operations inside the encoder_bridge_enable(). * call them in their (seemingly) original sequences - crtc_enable - encoder_bridge_enable(pre_enable) - encoder_bridge_enable(!pre_enable) #2 * rearrange the calls to re-order the operation - encoder_bridge_enable(pre_enable) - crtc_enable - encoder_bridge_enable(!pre_enable) This would have made the patch#2 seem quite trivial, as patch#1 would take the brunt of changes, while keeping the functionality intact. What I have now realized is that, the above isn't possible, unfortunately. The moment we separate out pre_enable and enable into 2 different calls, the overall sequence gets even changed when there are multiple pipelines in the system. So to implement the split properly, the first patch would look like this #1 * refactoring of entire loops into separate functions. * The calling sequences will be as follows - - crtc_enable() - encoder_bridge_enable() - this will do both pre_enable and enable unconditionally. The pre_enable and enable operations wouldn't be separated in patch 1, just that the crtc enable and encoder bridge pre_enable/enable loops would be put into individual functions. The next patch will then introduce booleans, and separate out pre_enable and enable, and implement the new order - #2 * pre_enable and enable operations will be conditionally segregated inside encoder_bridge_enable(), based on the pre_enable boolean. * The calling sequences will then be updated to - - encoder_bridge_enable(pre_enable) - crtc_enable() - encoder_bridge_enable(!pre_enable) This wouldn't be all that much trivial as I had imagined it to be earlier. It would also *kind of* like these patches in a previous revision, v5:11/13 [0], and v5:12/13 [1]. The only differences being, 1) these older patches separated out only the bridge/encoder operations into a function, and not the crtc operations, and 2) An enum is being used instead of the booleans. I believe this is what you are looking for? If I have misunderstood something, do let me know. Regards Aradhya [0]: v5:11/13 drm/atomic-helper: Separate out Encoder-Bridge enable and disable https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/ [1]: v5:12/13 drm/atomic-helper: Re-order bridge chain pre-enable and post-disable https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/