From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lelv0142.ext.ti.com (lelv0142.ext.ti.com [198.47.23.249]) (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 98CCC18396D for ; Thu, 30 May 2024 09:44:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.47.23.249 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717062267; cv=none; b=UXml2hrXbiM++YMzmaLD9AM4MSRHbH0T1V8mPZeZJB+SB5uXkI6XKQTsavWnWMpN5QCnhypKTdxOik0iZBRQxjC17MnxDOOGqiU8aeO11fPBXRb3kOkOJs+Kz3Bl8eZ4v3lzeqBA5QI4ZoBbzDjKp8/eqUy/8FXEbd9QjnXBHGg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717062267; c=relaxed/simple; bh=Ah8T+DbZaMK310fcA0Lv4Scw3Ix1e0dohjdOJ3aSqj0=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=lYKh2YFImuzwKT5pRAAU+7eJ1ns3OJZXRjjKFeAW4Zxm1oMYXTWrKBXX/wizP0xVMVxPzjK+R+Z3YyxG86Yo1lXpmxLb2Bsb8EtbJaTza5q/OyryKWuYO4VVE5pq68LHbafI9/hSK/HAFmSWGnRlylkMWAED7qlyFvg4AEbyTiA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com; spf=pass smtp.mailfrom=ti.com; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b=XGNk4hJY; arc=none smtp.client-ip=198.47.23.249 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ti.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b="XGNk4hJY" Received: from lelv0266.itg.ti.com ([10.180.67.225]) by lelv0142.ext.ti.com (8.15.2/8.15.2) with ESMTP id 44U9hplk108023; Thu, 30 May 2024 04:43:51 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1717062231; bh=cVkiqLAx26wTUKDADQCPLftPp08NYweVhmUaWKz3dhg=; h=Date:Subject:To:CC:References:From:In-Reply-To; b=XGNk4hJYYWvoixVg968XV3BYNcrzC1Rp1FBtMC3SKjEkHLykpAOPj8E04/sUg9pRX iUqKjPgYaGR9YoxtDBeAI3dV4gPq0cf3QJvPwlag8wmEy/4CXKUJgRpwlzpaAErm/N MRVcmvKohTFWM8lD/QsqhJlrk2GeBdEpx41u1ENY= Received: from DLEE104.ent.ti.com (dlee104.ent.ti.com [157.170.170.34]) by lelv0266.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 44U9hpPf108094 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 30 May 2024 04:43:51 -0500 Received: from DLEE100.ent.ti.com (157.170.170.30) by DLEE104.ent.ti.com (157.170.170.34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Thu, 30 May 2024 04:43:50 -0500 Received: from lelvsmtp5.itg.ti.com (10.180.75.250) by DLEE100.ent.ti.com (157.170.170.30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Thu, 30 May 2024 04:43:50 -0500 Received: from [172.24.227.31] (uda0496377.dhcp.ti.com [172.24.227.31]) by lelvsmtp5.itg.ti.com (8.15.2/8.15.2) with ESMTP id 44U9hhQV010017; Thu, 30 May 2024 04:43:43 -0500 Message-ID: Date: Thu, 30 May 2024 15:13:42 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable To: Maxime Ripard CC: Tomi Valkeinen , Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , Maarten Lankhorst , Jyri Sarha , Thomas Zimmermann , David Airlie , Daniel Vetter , DRI Development List , Linux Kernel List , Sam Ravnborg , Thierry Reding , Kieran Bingham , Boris Brezillon , Nishanth Menon , Vignesh Raghavendra , Praneeth Bajjuri , Udit Kumar , Devarsh Thakkar , Jayesh Choudhary , Jai Luthra References: <20240511153051.1355825-1-a-bhatia1@ti.com> <20240511153051.1355825-7-a-bhatia1@ti.com> <20240516-bipedal-keen-taipan-eedbe7@penduick> <20240521-realistic-imposing-lemur-aac3ad@houat> <1a6b8a83-b378-4869-b536-0fca76e428bf@ti.com> <20240528-encouraging-gray-lionfish-54ca83@houat> Content-Language: en-US From: Aradhya Bhatia In-Reply-To: <20240528-encouraging-gray-lionfish-54ca83@houat> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Hi Maxime, On 28/05/24 17:13, Maxime Ripard wrote: > On Fri, May 24, 2024 at 04:38:13PM GMT, Aradhya Bhatia wrote: >> Hi Maxime, >> >> On 21/05/24 18:45, Maxime Ripard wrote: >>> Hi, >>> >>> On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote: >>>>>> /** >>>>>> * @pre_enable: >>>>>> * >>>>>> @@ -285,6 +319,26 @@ struct drm_bridge_funcs { >>>>>> */ >>>>>> void (*enable)(struct drm_bridge *bridge); >>>>>> >>>>>> + /** >>>>>> + * @atomic_early_enable: >>>>>> + * >>>>>> + * This callback should enable the bridge. It is called right before >>>>>> + * the preceding element in the display pipe is enabled. If the >>>>>> + * preceding element is a bridge this means it's called before that >>>>>> + * bridge's @atomic_early_enable. If the preceding element is a >>>>>> + * &drm_crtc it's called right before the crtc's >>>>>> + * &drm_crtc_helper_funcs.atomic_enable hook. >>>>>> + * >>>>>> + * The display pipe (i.e. clocks and timing signals) feeding this bridge >>>>>> + * will not yet be running when this callback is called. The bridge can >>>>>> + * enable the display link feeding the next bridge in the chain (if >>>>>> + * there is one) when this callback is called. >>>>>> + * >>>>>> + * The @early_enable callback is optional. >>>>>> + */ >>>>>> + void (*atomic_early_enable)(struct drm_bridge *bridge, >>>>>> + struct drm_bridge_state *old_bridge_state); >>>>>> + >>>>>> /** >>>>>> * @atomic_pre_enable: >>>>>> * >>>>>> @@ -361,6 +415,21 @@ struct drm_bridge_funcs { >>>>>> void (*atomic_post_disable)(struct drm_bridge *bridge, >>>>>> struct drm_bridge_state *old_bridge_state); >>>>>> >>>>>> + /** >>>>>> + * @atomic_late_disable: >>>>>> + * >>>>>> + * This callback should disable the bridge. It is called right after the >>>>>> + * preceding element in the display pipe is disabled. If the preceding >>>>>> + * element is a bridge this means it's called after that bridge's >>>>>> + * @atomic_late_disable. If the preceding element is a &drm_crtc it's >>>>>> + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable >>>>>> + * hook. >>>>>> + * >>>>>> + * The @atomic_late_disable callback is optional. >>>>>> + */ >>>>>> + void (*atomic_late_disable)(struct drm_bridge *bridge, >>>>>> + struct drm_bridge_state *old_bridge_state); >>>>>> + >>>>> >>>>> But more importantly, I don't quite get the use case you're trying to >>>>> solve here. >>>>> >>>>> If I got the rest of your series, the Cadence DSI bridge needs to be >>>>> powered up before its source is started. You can't use atomic_enable or >>>>> atomic_pre_enable because it would start the source before the DSI >>>>> bridge. Is that correct? >>>>> >>>> >>>> That's right. I cannot use bridge_atomic_pre_enable / >>>> bridge_atomic_enable here. But that's because my source is CRTC, which >>>> gets enabled via crtc_atomic_enable. >>>> >>>> >>>>> If it is, then how is it different from what >>>>> drm_atomic_bridge_chain_pre_enable is doing? The assumption there is >>>>> that it starts enabling bridges last to first, to it should be enabled >>>>> before anything starts. >>>>> >>>>> The whole bridge enabling order code starts to be a bit of a mess, so it >>>>> would be great if you could list all the order variations we have >>>>> currently, and why none work for cdns-dsi. >>>>> >>>> >>>> Of course! I can elaborate on the order. >>>> >>>> Without my patches (and given there isn't any bridge setting the >>>> "pre_enable_prev_first" flag) the order of enable for any single display >>>> chain, looks like this - >>>> >>>> crtc_enable >>>> >>>> bridge[n]_pre_enable >>>> --- >>>> bridge[1]_pre_enable >>>> >>>> encoder_enable >>>> >>>> bridge[1]_enable >>>> --- >>>> bridge[n]_enable >>>> >>>> The tidss enables at the crtc_enable level, and hence is the first >>>> entity with stream on. cdns-dsi doesn't stand a chance with >>>> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no >>>> bridge call happening before crtc currently. >>> >>> Thanks for filling the blanks :) >>> >>> I assume that since cdns-dsi is a bridge, and it only has a simple >>> encoder implementation, for it to receive some video signal we need to >>> enable the CRTC before the bridge. >>> >>> If so, I think that's the original intent between the bridge pre_enable. >>> The original documentation had: >>> >>> pre_enable: this contains things needed to be done for the bridge >>> before this contains things needed to be done for the bridge before >>> this contains things needed to be done for the bridge before. >>> >>> and the current one has: >>> >>> The display pipe (i.e. clocks and timing signals) feeding this bridge >>> will not yet be running when this callback is called. The bridge must >>> not enable the display link feeding the next bridge in the chain (if >>> there is one) when this callback is called. >>> >>> I would say the CRTC is such a source, even more so now that the encoder >>> is usually transparent, so I think we should instead move the crtc >>> enable call after the bridge pre_enable. >> >> Hmm, if I understand you right, the newer sequence of calls will look >> like this, >> >> bridge[n]_pre_enable >> --- >> bridge[1]_pre_enable >> >> crtc_enable >> encoder_enable >> >> bridge[1]_enable >> --- >> bridge[n]_enable > > Yes :) > >> I do agree with this. This makes sense. CRTC is indeed such a source, >> and should ideally be enabled after the bridges are pre_enabled. >> >>> >>> Would that work? >>> >> >> So, this could potentially work, yes. The cdns-dsi would get pre_enabled >> after all bridges after cdns-dsi are pre_enabled. But over a quick test >> with BBAI64 + RPi Panel, I don't see any issue. >> >> However, the one concern that I have right now, is about breaking any >> existing (albeit faulty) implementation which relies on CRTC being >> enabled before the bridges are pre_enabled. =) > > I don't think it'll be a big deal. If there was a proper encoder driver, > it was probably gating the signal until it's enabled. If there isn't, > then yeah it might disrupt things, but it mostly means that the driver > wasn't properly split between pre_enable and enable. > > So I think it's worth trying, and we'll see the outcome. > Alright! =) Have made the changes as per your suggestions in v2. Thanks! Regards Aradhya