From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-188.mta1.migadu.com (out-188.mta1.migadu.com [95.215.58.188]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1820C1FF7D6 for ; Fri, 17 Jan 2025 13:07:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.188 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737119278; cv=none; b=LAI3zUFnHPCDfwnewnDLI9SrJiYH2IDS9NIgofKj9MvwZ+pkU9pz3nIPhdYFVrPLoKlnKa11Kza6XMMIKawwEDkRjhOM0zKoq9YPNZi2v5hD7N1S9+HkcW8Tzx4xeqe9t719rJAnKP6MMjcyIfUFiMtqSuW5THxTTsVA5bNfx1I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737119278; c=relaxed/simple; bh=/Hw/TYb4JtSIQMLL1usNZ5Ojh60mLSo4l6e2g9RkjRc=; h=Message-ID:Date:MIME-Version:From:Subject:To:Cc:References: In-Reply-To:Content-Type; b=BAhspKlMTCwhM5eNNjF8lid0CvakO/NKJfuDcxL3V5Ofxuwnz1ifJn01ssafqDQGs0jXyK5ujCcXI6uAZTNnJMYd0URh6qtzoV3Yi3RpHp/J6uxW/ZfIGBSxvRkDFRUX8+QE+JN5e9wws6V+m9v99NVCTFB19hTRoW6l5BwPIv0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=pArEjGXs; arc=none smtp.client-ip=95.215.58.188 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="pArEjGXs" 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 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: 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 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/