From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-176.mta1.migadu.com (out-176.mta1.migadu.com [95.215.58.176]) (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 C7C0E1F542D for ; Tue, 21 Jan 2025 17:43:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.176 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737481425; cv=none; b=uTIQZGhASbeq43YYzCzcrR/OmsATDq1CpbafPSiNo+Io5/E2laAPUcFnkhf6gqpny1sotmjZlDUNIb25WG4hNWZSdiylNVfKdT28GQx4Dk34yr/d0gWBEAD1b+fpDaDwCZwLfw3Q6SoXXKSQ9+aOZpo5UUD/XwTRAEf0TxdlFRE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737481425; c=relaxed/simple; bh=aNwtwMkEqI3RLfl9cSWgL9Bal8iJwejvNeGUIEX+PhE=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=U6th9k9wVA2AHBnmCaHgSRMAjYYB2c9nf4ix0fuoZvywpP7wISwhvs5ZA5hewtPgmvaoD83pj87TsbJeqfDj6Ww510NbtOVzUMyh05Y7Du3zgBEGDZ8Q4rLl010qoTBWGt+EAvnB470QxGiFnRgLg8MEgtb1mH24bqQFKCf4p1E= 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=mrlW+tAb; arc=none smtp.client-ip=95.215.58.176 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="mrlW+tAb" Message-ID: <506789d5-b398-43d0-93f2-6947bcc2c6d6@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1737481419; 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=L2kdb3klZjvX15URtdvUVT6YXANgSBlPylcft3gNYSw=; b=mrlW+tAbZRCmzMg1U1rupknJhRX+HdZMj0v+0ZLqWY3WwdJ4Ax0UIKwSjP7hKcdJ8PicpM 8OL8jbEBRPl2G05gR5gJcCVKvCcMO5EmWZm4w0g2fGSzDUVZeo1Xrh/LyKjj1US/p88Zp2 oxPGbqL9J3oMhVXoq7VqSJpSv9OTWqs= Date: Tue, 21 Jan 2025 23:12:46 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 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> <30dc847b-7b3b-4c6c-be10-b941f6acf4b9@linux.dev> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Aradhya Bhatia In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT Hi Dmitry, On 21/01/25 16:20, Dmitry Baryshkov wrote: > On Mon, Jan 20, 2025 at 11:18:22PM +0530, Aradhya Bhatia wrote: >> Hi Dmitry, >> >> On 20/01/25 14:08, Dmitry Baryshkov wrote: >>> On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote: >>>> 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) >>> >>> >>> I'd say slightly differently: >>> >>> #1 Refactor loops into separate functions: >>> - crtc_enable() >>> - encoder_bridge_enable(), loops over encoders and calls >>> pre_enable(bridges), enable(encoder), enable(bridges) >>> >>> #2 Split loops into separate functions: >>> - crtc_enable() >>> - encoder_bridge_pre_enable(), loops over encoders and calls >>> pre_enable(bridges), >>> - encoder_bridge_enable(), loops over encoders and calls >>> enable(encoder), enable(bridges) >>> >> >> When we consider setups with multiple independent displays, there are >> often multiple crtcs in the system, each with their own encoder-bridge >> chain. >> >> In such a scenario, the sequence of crtc/encoder/bridge enable calls >> after the #2 that you suggested, will not the remain same as that after >> #1 (which is the _original_ sequence of calls). > > Yes. The order of ops between display has changed, but each display is > still programmed in exactly the same way as before. Yes, that's right. Sequence for each display will remain the same as before. > >> >> Do we still require #2 as an intermediate step between the original >> sequence, and the intended new sequence? Wouldn't having the sequence >> change in 2 half-steps add to the confusion (should some driver break >> due to either of the refactorings)? > > That's the point. Having two refactorings in one commit makes it harder > to understand, which one broke the driver. Having two separate commits > allows users to revert the latter commit and check what caused the > issue. Right. It's easier to debug each display independently in multi-display setups, and I can now understand how #2 will be able to help. This explanation helped a lot. Thank you! >> >>> #3 Move crtc_enable() calls: >>> - encoder_bridge_pre_enable(), loops over encoders and calls >>> pre_enable(bridges), >>> - crtc_enable() >>> - encoder_bridge_enable(), loops over encoders and calls >>> enable(encoder), enable(bridges) >>> >>> You might use enum or booleans to implement encoder_bridge_pre_enable(), >>> or just keep it as a completely separate function (might be a better >>> option). >> >> Yeah, I suppose a separate function may be better. There will, however, >> be some code duplication when we loop over the encoder twice, once for >> pre_enable(bridge) and the other for enable(encoder) and enable(bridge). >> >> I hope that will be acceptable? > > I'd prefer two separate functions, but I won't insist on that. Alright! I have my work cut out for me for the next revision. > >> >>> >>> The reason why I'm suggesting it is pretty easy: your patch performs two >>> refactorings at the same time. If one of the drivers breaks, we have no >>> way to identify, which of the refactorings caused the break.> >>>> >>>> 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/ >>>> >>> >> >> >> Regards >> Aradhya >> > -- Regards Aradhya