From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753130AbeDFP2x (ORCPT ); Fri, 6 Apr 2018 11:28:53 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:12283 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752858AbeDFP2s (ORCPT ); Fri, 6 Apr 2018 11:28:48 -0400 From: Philippe CORNU To: Laurent Pinchart CC: Daniel Vetter , Gustavo Padovan , Maarten Lankhorst , Sean Paul , David Airlie , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "Benjamin Gaignard" , Yannick FERTRE , Vincent ABRIOU Subject: Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc Thread-Topic: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc Thread-Index: AQHTzbb6Xc81Ni58ZkqJDtQmzYCJWqPzutsA Date: Fri, 6 Apr 2018 15:28:27 +0000 Message-ID: References: <20180226121605.12050-1-philippe.cornu@st.com> <1765089.gI5krnOhAB@avalon> In-Reply-To: <1765089.gI5krnOhAB@avalon> Accept-Language: fr-FR, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.50] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-04-06_08:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id w36FSxEf022992 Hi Laurent, On 04/06/2018 04:53 PM, Laurent Pinchart wrote: > Hi Philippe, > > Thank you for the patch. > > On Monday, 26 February 2018 14:16:04 EEST Philippe Cornu wrote: >> This patch clarifies the adjusted_mode documentation >> for a bridge directly connected to a crtc. >> >> Signed-off-by: Philippe Cornu >> --- >> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367 >> >> include/drm/drm_bridge.h | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >> index 3270fec46979..b5f3c070467c 100644 >> --- a/include/drm/drm_bridge.h >> +++ b/include/drm/drm_bridge.h >> @@ -177,7 +177,8 @@ struct drm_bridge_funcs { >> * pipeline has been called already. If the bridge is the first element >> * then this would be &drm_encoder_helper_funcs.mode_set. The display >> * pipe (i.e. clocks and timing signals) is off when this function is >> - * called. >> + * called. If the bridge is connected to the crtc, the adjusted_mode >> + * parameter is the one defined in &drm_crtc_state.adjusted_mode. > > Unless I'm mistaken this will always be the mode stored in > &drm_crtc_state.adjusted_mode (at least for atomic drivers), regardless of > whether the bridge is the first in the chain (connected to the CRTC) or not. > What is important to document is that we have a single adjusted_mode for the > whole chain of bridges, and that it corresponds to the mode output by the CRTC > for the first bridge. Bridges further in the chain can look at that mode, > although there will probably be nothing of interest to them there. > > How about the following text ? > > /** > * @mode_set: > * > * This callback should set the given mode on the bridge. It is called > * after the @mode_set callback for the preceding element in the display > * pipeline has been called already. If the bridge is the first element > * then this would be &drm_encoder_helper_funcs.mode_set. The display > * pipe (i.e. clocks and timing signals) is off when this function is > * called. > * > * The adjusted_mode parameter corresponds to the mode output by the CRTC > * for the first bridge in the chain. It can be different from the mode > * parameter that contains the desired mode for the connector at the end > * of the bridges chain, for instance when the first bridge in the chain > * performs scaling. The adjusted mode is mostly useful for the first > * bridge in the chain and is likely irrelevant for the other bridges. > * > * For atomic drivers the adjusted_mode is the mode stored in > * &drm_crtc_state.adjusted_mode. > * > * NOTE: > * > * If a need arises to store and access modes adjusted for other locations > * than the connection between the CRTC and the first bridge, the DRM > * framework will have to be extended with DRM bridge states. > */ > > Then I think we should also update the documentation of > drm_crtc_state.adjusted_mode accordingly: > > /* > * @adjusted_mode: > * > * Internal display timings which can be used by the driver to handle > * differences between the mode requested by userspace in @mode and what > * is actually programmed into the hardware. > * > * For drivers using drm_bridge, this store the hardware display timings > * used between the CRTC and the first bridge. For other drivers, the > * meaning of the adjusted_mode field is purely driver implementation > * defined information, and will usually be used to store the hardware > * display timings used between the CRTC and encoder blocks. > */ > Your proposal is very clear and understandable. I will make a new patch version based on it. Big thanks Philippe :-)