From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc Date: Mon, 9 Apr 2018 09:56:32 +0200 Message-ID: <20180409075632.GC31310@phenom.ffwll.local> References: <20180226121605.12050-1-philippe.cornu@st.com> <1765089.gI5krnOhAB@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wm0-x235.google.com (mail-wm0-x235.google.com [IPv6:2a00:1450:400c:c09::235]) by gabe.freedesktop.org (Postfix) with ESMTPS id 79C476E025 for ; Mon, 9 Apr 2018 07:56:36 +0000 (UTC) Received: by mail-wm0-x235.google.com with SMTP id i3so14548635wmf.3 for ; Mon, 09 Apr 2018 00:56:36 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Philippe CORNU Cc: David Airlie , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , Yannick FERTRE , Laurent Pinchart , Daniel Vetter , Vincent ABRIOU List-Id: dri-devel@lists.freedesktop.org T24gRnJpLCBBcHIgMDYsIDIwMTggYXQgMDM6Mjg6MjdQTSArMDAwMCwgUGhpbGlwcGUgQ09STlUg d3JvdGU6Cj4gSGkgTGF1cmVudCwKPiAKPiBPbiAwNC8wNi8yMDE4IDA0OjUzIFBNLCBMYXVyZW50 IFBpbmNoYXJ0IHdyb3RlOgo+ID4gSGkgUGhpbGlwcGUsCj4gPiAKPiA+IFRoYW5rIHlvdSBmb3Ig dGhlIHBhdGNoLgo+ID4gCj4gPiBPbiBNb25kYXksIDI2IEZlYnJ1YXJ5IDIwMTggMTQ6MTY6MDQg RUVTVCBQaGlsaXBwZSBDb3JudSB3cm90ZToKPiA+PiBUaGlzIHBhdGNoIGNsYXJpZmllcyB0aGUg YWRqdXN0ZWRfbW9kZSBkb2N1bWVudGF0aW9uCj4gPj4gZm9yIGEgYnJpZGdlIGRpcmVjdGx5IGNv bm5lY3RlZCB0byBhIGNydGMuCj4gPj4KPiA+PiBTaWduZWQtb2ZmLWJ5OiBQaGlsaXBwZSBDb3Ju dSA8cGhpbGlwcGUuY29ybnVAc3QuY29tPgo+ID4+IC0tLQo+ID4+IFRoaXMgcGF0Y2ggaXMgbGlu a2VkIHRvIHRoZSBkaXNjdXNzaW9uIGh0dHBzOi8vbGttbC5vcmcvbGttbC8yMDE4LzEvMjUvMzY3 Cj4gPj4KPiA+PiAgIGluY2x1ZGUvZHJtL2RybV9icmlkZ2UuaCB8IDMgKystCj4gPj4gICAxIGZp bGUgY2hhbmdlZCwgMiBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCj4gPj4KPiA+PiBkaWZm IC0tZ2l0IGEvaW5jbHVkZS9kcm0vZHJtX2JyaWRnZS5oIGIvaW5jbHVkZS9kcm0vZHJtX2JyaWRn ZS5oCj4gPj4gaW5kZXggMzI3MGZlYzQ2OTc5Li5iNWYzYzA3MDQ2N2MgMTAwNjQ0Cj4gPj4gLS0t IGEvaW5jbHVkZS9kcm0vZHJtX2JyaWRnZS5oCj4gPj4gKysrIGIvaW5jbHVkZS9kcm0vZHJtX2Jy aWRnZS5oCj4gPj4gQEAgLTE3Nyw3ICsxNzcsOCBAQCBzdHJ1Y3QgZHJtX2JyaWRnZV9mdW5jcyB7 Cj4gPj4gICAJICogcGlwZWxpbmUgaGFzIGJlZW4gY2FsbGVkIGFscmVhZHkuIElmIHRoZSBicmlk Z2UgaXMgdGhlIGZpcnN0IGVsZW1lbnQKPiA+PiAgIAkgKiB0aGVuIHRoaXMgd291bGQgYmUgJmRy bV9lbmNvZGVyX2hlbHBlcl9mdW5jcy5tb2RlX3NldC4gVGhlIGRpc3BsYXkKPiA+PiAgIAkgKiBw aXBlIChpLmUuICBjbG9ja3MgYW5kIHRpbWluZyBzaWduYWxzKSBpcyBvZmYgd2hlbiB0aGlzIGZ1 bmN0aW9uIGlzCj4gPj4gLQkgKiBjYWxsZWQuCj4gPj4gKwkgKiBjYWxsZWQuIElmIHRoZSBicmlk Z2UgaXMgY29ubmVjdGVkIHRvIHRoZSBjcnRjLCB0aGUgYWRqdXN0ZWRfbW9kZQo+ID4+ICsJICog cGFyYW1ldGVyIGlzIHRoZSBvbmUgZGVmaW5lZCBpbiAmZHJtX2NydGNfc3RhdGUuYWRqdXN0ZWRf bW9kZS4KPiA+IAo+ID4gVW5sZXNzIEknbSBtaXN0YWtlbiB0aGlzIHdpbGwgYWx3YXlzIGJlIHRo ZSBtb2RlIHN0b3JlZCBpbgo+ID4gJmRybV9jcnRjX3N0YXRlLmFkanVzdGVkX21vZGUgKGF0IGxl YXN0IGZvciBhdG9taWMgZHJpdmVycyksIHJlZ2FyZGxlc3Mgb2YKPiA+IHdoZXRoZXIgdGhlIGJy aWRnZSBpcyB0aGUgZmlyc3QgaW4gdGhlIGNoYWluIChjb25uZWN0ZWQgdG8gdGhlIENSVEMpIG9y IG5vdC4KPiA+IFdoYXQgaXMgaW1wb3J0YW50IHRvIGRvY3VtZW50IGlzIHRoYXQgd2UgaGF2ZSBh IHNpbmdsZSBhZGp1c3RlZF9tb2RlIGZvciB0aGUKPiA+IHdob2xlIGNoYWluIG9mIGJyaWRnZXMs IGFuZCB0aGF0IGl0IGNvcnJlc3BvbmRzIHRvIHRoZSBtb2RlIG91dHB1dCBieSB0aGUgQ1JUQwo+ ID4gZm9yIHRoZSBmaXJzdCBicmlkZ2UuIEJyaWRnZXMgZnVydGhlciBpbiB0aGUgY2hhaW4gY2Fu IGxvb2sgYXQgdGhhdCBtb2RlLAo+ID4gYWx0aG91Z2ggdGhlcmUgd2lsbCBwcm9iYWJseSBiZSBu b3RoaW5nIG9mIGludGVyZXN0IHRvIHRoZW0gdGhlcmUuCj4gPiAKPiA+IEhvdyBhYm91dCB0aGUg Zm9sbG93aW5nIHRleHQgPwo+ID4gCj4gPiAgICAgIC8qKgo+ID4gICAgICAgKiBAbW9kZV9zZXQ6 Cj4gPiAgICAgICAqCj4gPiAgICAgICAqIFRoaXMgY2FsbGJhY2sgc2hvdWxkIHNldCB0aGUgZ2l2 ZW4gbW9kZSBvbiB0aGUgYnJpZGdlLiBJdCBpcyBjYWxsZWQKPiA+ICAgICAgICogYWZ0ZXIgdGhl IEBtb2RlX3NldCBjYWxsYmFjayBmb3IgdGhlIHByZWNlZGluZyBlbGVtZW50IGluIHRoZSBkaXNw bGF5Cj4gPiAgICAgICAqIHBpcGVsaW5lIGhhcyBiZWVuIGNhbGxlZCBhbHJlYWR5LiBJZiB0aGUg YnJpZGdlIGlzIHRoZSBmaXJzdCBlbGVtZW50Cj4gPiAgICAgICAqIHRoZW4gdGhpcyB3b3VsZCBi ZSAmZHJtX2VuY29kZXJfaGVscGVyX2Z1bmNzLm1vZGVfc2V0LiBUaGUgZGlzcGxheQo+ID4gICAg ICAgKiBwaXBlIChpLmUuICBjbG9ja3MgYW5kIHRpbWluZyBzaWduYWxzKSBpcyBvZmYgd2hlbiB0 aGlzIGZ1bmN0aW9uIGlzCj4gPiAgICAgICAqIGNhbGxlZC4KPiA+ICAgICAgICoKPiA+ICAgICAg ICogVGhlIGFkanVzdGVkX21vZGUgcGFyYW1ldGVyIGNvcnJlc3BvbmRzIHRvIHRoZSBtb2RlIG91 dHB1dCBieSB0aGUgQ1JUQwo+ID4gICAgICAgKiBmb3IgdGhlIGZpcnN0IGJyaWRnZSBpbiB0aGUg Y2hhaW4uIEl0IGNhbiBiZSBkaWZmZXJlbnQgZnJvbSB0aGUgbW9kZQo+ID4gICAgICAgKiBwYXJh bWV0ZXIgdGhhdCBjb250YWlucyB0aGUgZGVzaXJlZCBtb2RlIGZvciB0aGUgY29ubmVjdG9yIGF0 IHRoZSBlbmQKPiA+ICAgICAgICogb2YgdGhlIGJyaWRnZXMgY2hhaW4sIGZvciBpbnN0YW5jZSB3 aGVuIHRoZSBmaXJzdCBicmlkZ2UgaW4gdGhlIGNoYWluCj4gPiAgICAgICAqIHBlcmZvcm1zIHNj YWxpbmcuIFRoZSBhZGp1c3RlZCBtb2RlIGlzIG1vc3RseSB1c2VmdWwgZm9yIHRoZSBmaXJzdAo+ ID4gICAgICAgKiBicmlkZ2UgaW4gdGhlIGNoYWluIGFuZCBpcyBsaWtlbHkgaXJyZWxldmFudCBm b3IgdGhlIG90aGVyIGJyaWRnZXMuCj4gPiAgICAgICAqCj4gPiAgICAgICAqIEZvciBhdG9taWMg ZHJpdmVycyB0aGUgYWRqdXN0ZWRfbW9kZSBpcyB0aGUgbW9kZSBzdG9yZWQgaW4KPiA+ICAgICAg ICogJmRybV9jcnRjX3N0YXRlLmFkanVzdGVkX21vZGUuCj4gPiAgICAgICAqCj4gPiAgICAgICAq IE5PVEU6Cj4gPiAgICAgICAqCj4gPiAgICAgICAqIElmIGEgbmVlZCBhcmlzZXMgdG8gc3RvcmUg YW5kIGFjY2VzcyBtb2RlcyBhZGp1c3RlZCBmb3Igb3RoZXIgbG9jYXRpb25zCj4gPiAgICAgICAq IHRoYW4gdGhlIGNvbm5lY3Rpb24gYmV0d2VlbiB0aGUgQ1JUQyBhbmQgdGhlIGZpcnN0IGJyaWRn ZSwgdGhlIERSTQo+ID4gICAgICAgKiBmcmFtZXdvcmsgd2lsbCBoYXZlIHRvIGJlIGV4dGVuZGVk IHdpdGggRFJNIGJyaWRnZSBzdGF0ZXMuCj4gPiAgICAgCSAqLwo+ID4gCj4gPiBUaGVuIEkgdGhp bmsgd2Ugc2hvdWxkIGFsc28gdXBkYXRlIHRoZSBkb2N1bWVudGF0aW9uIG9mCj4gPiBkcm1fY3J0 Y19zdGF0ZS5hZGp1c3RlZF9tb2RlIGFjY29yZGluZ2x5Ogo+ID4gCj4gPiAgICAgIC8qCj4gPiAg ICAgICAqIEBhZGp1c3RlZF9tb2RlOgo+ID4gICAgICAgKgo+ID4gICAgICAgKiBJbnRlcm5hbCBk aXNwbGF5IHRpbWluZ3Mgd2hpY2ggY2FuIGJlIHVzZWQgYnkgdGhlIGRyaXZlciB0byBoYW5kbGUK PiA+ICAgICAgICogZGlmZmVyZW5jZXMgYmV0d2VlbiB0aGUgbW9kZSByZXF1ZXN0ZWQgYnkgdXNl cnNwYWNlIGluIEBtb2RlIGFuZCB3aGF0Cj4gPiAgICAgICAqIGlzIGFjdHVhbGx5IHByb2dyYW1t ZWQgaW50byB0aGUgaGFyZHdhcmUuCj4gPiAgICAgICAqCj4gPiAgICAgICAqIEZvciBkcml2ZXJz IHVzaW5nIGRybV9icmlkZ2UsIHRoaXMgc3RvcmUgdGhlIGhhcmR3YXJlIGRpc3BsYXkgdGltaW5n cwo+ID4gICAgICAgKiB1c2VkIGJldHdlZW4gdGhlIENSVEMgYW5kIHRoZSBmaXJzdCBicmlkZ2Uu IEZvciBvdGhlciBkcml2ZXJzLCB0aGUKPiA+ICAgICAgICogbWVhbmluZyBvZiB0aGUgYWRqdXN0 ZWRfbW9kZSBmaWVsZCBpcyBwdXJlbHkgZHJpdmVyIGltcGxlbWVudGF0aW9uCj4gPiAgICAgICAq IGRlZmluZWQgaW5mb3JtYXRpb24sIGFuZCB3aWxsIHVzdWFsbHkgYmUgdXNlZCB0byBzdG9yZSB0 aGUgaGFyZHdhcmUKPiA+ICAgICAgICogZGlzcGxheSB0aW1pbmdzIHVzZWQgYmV0d2VlbiB0aGUg Q1JUQyBhbmQgZW5jb2RlciBibG9ja3MuCj4gPiAgICAgICAqLwo+ID4gCj4gCj4gWW91ciBwcm9w b3NhbCBpcyB2ZXJ5IGNsZWFyIGFuZCB1bmRlcnN0YW5kYWJsZS4gSSB3aWxsIG1ha2UgYSBuZXcg cGF0Y2ggCj4gdmVyc2lvbiBiYXNlZCBvbiBpdC4KCkp1c3QgdG8gYXZvaWQgY29uZnVzaW9uOiBO ZWVkcyB0byBiZSBhIGZ1bGx5IG5ldyBwYXRjaCBvbiB0b3Agb2YgbGF0ZXN0CmRybS1taXNjLW5l eHQsIHNpbmNlIG5vIHJlYmFzaW5nIGluIGEgZ3JvdXAgbWFpbnRhaW5lZCB0cmVlLgotRGFuaWVs Ci0tIApEYW5pZWwgVmV0dGVyClNvZnR3YXJlIEVuZ2luZWVyLCBJbnRlbCBDb3Jwb3JhdGlvbgpo dHRwOi8vYmxvZy5mZndsbC5jaApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVz a3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9k cmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751794AbeDIH4h (ORCPT ); Mon, 9 Apr 2018 03:56:37 -0400 Received: from mail-wm0-f46.google.com ([74.125.82.46]:37160 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750759AbeDIH4g (ORCPT ); Mon, 9 Apr 2018 03:56:36 -0400 X-Google-Smtp-Source: AIpwx4/uq0WAqA/ZHR3r//zvp3nfv2TAF9zwxE+9+a2h2rqJmGhiPpIZw1o+RPm4ku2whO2+nkh1Ww== Date: Mon, 9 Apr 2018 09:56:32 +0200 From: Daniel Vetter To: Philippe CORNU Cc: Laurent Pinchart , David Airlie , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , Yannick FERTRE , Daniel Vetter , Vincent ABRIOU Subject: Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc Message-ID: <20180409075632.GC31310@phenom.ffwll.local> Mail-Followup-To: Philippe CORNU , Laurent Pinchart , David Airlie , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , Yannick FERTRE , Daniel Vetter , Vincent ABRIOU References: <20180226121605.12050-1-philippe.cornu@st.com> <1765089.gI5krnOhAB@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 4.15.0-1-amd64 User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 06, 2018 at 03:28:27PM +0000, Philippe CORNU wrote: > 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. Just to avoid confusion: Needs to be a fully new patch on top of latest drm-misc-next, since no rebasing in a group maintained tree. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch