From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc Date: Fri, 06 Apr 2018 17:53:09 +0300 Message-ID: <1765089.gI5krnOhAB@avalon> References: <20180226121605.12050-1-philippe.cornu@st.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [185.26.127.97]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8606D6E97E for ; Fri, 6 Apr 2018 14:53:13 +0000 (UTC) In-Reply-To: <20180226121605.12050-1-philippe.cornu@st.com> 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 , Daniel Vetter , Vincent Abriou List-Id: dri-devel@lists.freedesktop.org SGkgUGhpbGlwcGUsCgpUaGFuayB5b3UgZm9yIHRoZSBwYXRjaC4KCk9uIE1vbmRheSwgMjYgRmVi cnVhcnkgMjAxOCAxNDoxNjowNCBFRVNUIFBoaWxpcHBlIENvcm51IHdyb3RlOgo+IFRoaXMgcGF0 Y2ggY2xhcmlmaWVzIHRoZSBhZGp1c3RlZF9tb2RlIGRvY3VtZW50YXRpb24KPiBmb3IgYSBicmlk Z2UgZGlyZWN0bHkgY29ubmVjdGVkIHRvIGEgY3J0Yy4KPiAKPiBTaWduZWQtb2ZmLWJ5OiBQaGls aXBwZSBDb3JudSA8cGhpbGlwcGUuY29ybnVAc3QuY29tPgo+IC0tLQo+IFRoaXMgcGF0Y2ggaXMg bGlua2VkIHRvIHRoZSBkaXNjdXNzaW9uIGh0dHBzOi8vbGttbC5vcmcvbGttbC8yMDE4LzEvMjUv MzY3Cj4gCj4gIGluY2x1ZGUvZHJtL2RybV9icmlkZ2UuaCB8IDMgKystCj4gIDEgZmlsZSBjaGFu Z2VkLCAyIGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkKPiAKPiBkaWZmIC0tZ2l0IGEvaW5j bHVkZS9kcm0vZHJtX2JyaWRnZS5oIGIvaW5jbHVkZS9kcm0vZHJtX2JyaWRnZS5oCj4gaW5kZXgg MzI3MGZlYzQ2OTc5Li5iNWYzYzA3MDQ2N2MgMTAwNjQ0Cj4gLS0tIGEvaW5jbHVkZS9kcm0vZHJt X2JyaWRnZS5oCj4gKysrIGIvaW5jbHVkZS9kcm0vZHJtX2JyaWRnZS5oCj4gQEAgLTE3Nyw3ICsx NzcsOCBAQCBzdHJ1Y3QgZHJtX2JyaWRnZV9mdW5jcyB7Cj4gIAkgKiBwaXBlbGluZSBoYXMgYmVl biBjYWxsZWQgYWxyZWFkeS4gSWYgdGhlIGJyaWRnZSBpcyB0aGUgZmlyc3QgZWxlbWVudAo+ICAJ ICogdGhlbiB0aGlzIHdvdWxkIGJlICZkcm1fZW5jb2Rlcl9oZWxwZXJfZnVuY3MubW9kZV9zZXQu IFRoZSBkaXNwbGF5Cj4gIAkgKiBwaXBlIChpLmUuICBjbG9ja3MgYW5kIHRpbWluZyBzaWduYWxz KSBpcyBvZmYgd2hlbiB0aGlzIGZ1bmN0aW9uIGlzCj4gLQkgKiBjYWxsZWQuCj4gKwkgKiBjYWxs ZWQuIElmIHRoZSBicmlkZ2UgaXMgY29ubmVjdGVkIHRvIHRoZSBjcnRjLCB0aGUgYWRqdXN0ZWRf bW9kZQo+ICsJICogcGFyYW1ldGVyIGlzIHRoZSBvbmUgZGVmaW5lZCBpbiAmZHJtX2NydGNfc3Rh dGUuYWRqdXN0ZWRfbW9kZS4KClVubGVzcyBJJ20gbWlzdGFrZW4gdGhpcyB3aWxsIGFsd2F5cyBi ZSB0aGUgbW9kZSBzdG9yZWQgaW4gCiZkcm1fY3J0Y19zdGF0ZS5hZGp1c3RlZF9tb2RlIChhdCBs ZWFzdCBmb3IgYXRvbWljIGRyaXZlcnMpLCByZWdhcmRsZXNzIG9mIAp3aGV0aGVyIHRoZSBicmlk Z2UgaXMgdGhlIGZpcnN0IGluIHRoZSBjaGFpbiAoY29ubmVjdGVkIHRvIHRoZSBDUlRDKSBvciBu b3QuIApXaGF0IGlzIGltcG9ydGFudCB0byBkb2N1bWVudCBpcyB0aGF0IHdlIGhhdmUgYSBzaW5n bGUgYWRqdXN0ZWRfbW9kZSBmb3IgdGhlIAp3aG9sZSBjaGFpbiBvZiBicmlkZ2VzLCBhbmQgdGhh dCBpdCBjb3JyZXNwb25kcyB0byB0aGUgbW9kZSBvdXRwdXQgYnkgdGhlIENSVEMgCmZvciB0aGUg Zmlyc3QgYnJpZGdlLiBCcmlkZ2VzIGZ1cnRoZXIgaW4gdGhlIGNoYWluIGNhbiBsb29rIGF0IHRo YXQgbW9kZSwgCmFsdGhvdWdoIHRoZXJlIHdpbGwgcHJvYmFibHkgYmUgbm90aGluZyBvZiBpbnRl cmVzdCB0byB0aGVtIHRoZXJlLgoKSG93IGFib3V0IHRoZSBmb2xsb3dpbmcgdGV4dCA/CgogICAg LyoqCiAgICAgKiBAbW9kZV9zZXQ6CiAgICAgKgogICAgICogVGhpcyBjYWxsYmFjayBzaG91bGQg c2V0IHRoZSBnaXZlbiBtb2RlIG9uIHRoZSBicmlkZ2UuIEl0IGlzIGNhbGxlZAogICAgICogYWZ0 ZXIgdGhlIEBtb2RlX3NldCBjYWxsYmFjayBmb3IgdGhlIHByZWNlZGluZyBlbGVtZW50IGluIHRo ZSBkaXNwbGF5CiAgICAgKiBwaXBlbGluZSBoYXMgYmVlbiBjYWxsZWQgYWxyZWFkeS4gSWYgdGhl IGJyaWRnZSBpcyB0aGUgZmlyc3QgZWxlbWVudAogICAgICogdGhlbiB0aGlzIHdvdWxkIGJlICZk cm1fZW5jb2Rlcl9oZWxwZXJfZnVuY3MubW9kZV9zZXQuIFRoZSBkaXNwbGF5CiAgICAgKiBwaXBl IChpLmUuICBjbG9ja3MgYW5kIHRpbWluZyBzaWduYWxzKSBpcyBvZmYgd2hlbiB0aGlzIGZ1bmN0 aW9uIGlzCiAgICAgKiBjYWxsZWQuCiAgICAgKgogICAgICogVGhlIGFkanVzdGVkX21vZGUgcGFy YW1ldGVyIGNvcnJlc3BvbmRzIHRvIHRoZSBtb2RlIG91dHB1dCBieSB0aGUgQ1JUQwogICAgICog Zm9yIHRoZSBmaXJzdCBicmlkZ2UgaW4gdGhlIGNoYWluLiBJdCBjYW4gYmUgZGlmZmVyZW50IGZy b20gdGhlIG1vZGUKICAgICAqIHBhcmFtZXRlciB0aGF0IGNvbnRhaW5zIHRoZSBkZXNpcmVkIG1v ZGUgZm9yIHRoZSBjb25uZWN0b3IgYXQgdGhlIGVuZAogICAgICogb2YgdGhlIGJyaWRnZXMgY2hh aW4sIGZvciBpbnN0YW5jZSB3aGVuIHRoZSBmaXJzdCBicmlkZ2UgaW4gdGhlIGNoYWluCiAgICAg KiBwZXJmb3JtcyBzY2FsaW5nLiBUaGUgYWRqdXN0ZWQgbW9kZSBpcyBtb3N0bHkgdXNlZnVsIGZv ciB0aGUgZmlyc3QKICAgICAqIGJyaWRnZSBpbiB0aGUgY2hhaW4gYW5kIGlzIGxpa2VseSBpcnJl bGV2YW50IGZvciB0aGUgb3RoZXIgYnJpZGdlcy4KICAgICAqCiAgICAgKiBGb3IgYXRvbWljIGRy aXZlcnMgdGhlIGFkanVzdGVkX21vZGUgaXMgdGhlIG1vZGUgc3RvcmVkIGluCiAgICAgKiAmZHJt X2NydGNfc3RhdGUuYWRqdXN0ZWRfbW9kZS4KICAgICAqCiAgICAgKiBOT1RFOgogICAgICoKICAg ICAqIElmIGEgbmVlZCBhcmlzZXMgdG8gc3RvcmUgYW5kIGFjY2VzcyBtb2RlcyBhZGp1c3RlZCBm b3Igb3RoZXIgbG9jYXRpb25zCiAgICAgKiB0aGFuIHRoZSBjb25uZWN0aW9uIGJldHdlZW4gdGhl IENSVEMgYW5kIHRoZSBmaXJzdCBicmlkZ2UsIHRoZSBEUk0KICAgICAqIGZyYW1ld29yayB3aWxs IGhhdmUgdG8gYmUgZXh0ZW5kZWQgd2l0aCBEUk0gYnJpZGdlIHN0YXRlcy4KICAgCSAqLwoKVGhl biBJIHRoaW5rIHdlIHNob3VsZCBhbHNvIHVwZGF0ZSB0aGUgZG9jdW1lbnRhdGlvbiBvZiAKZHJt X2NydGNfc3RhdGUuYWRqdXN0ZWRfbW9kZSBhY2NvcmRpbmdseToKCiAgICAvKgogICAgICogQGFk anVzdGVkX21vZGU6CiAgICAgKgogICAgICogSW50ZXJuYWwgZGlzcGxheSB0aW1pbmdzIHdoaWNo IGNhbiBiZSB1c2VkIGJ5IHRoZSBkcml2ZXIgdG8gaGFuZGxlCiAgICAgKiBkaWZmZXJlbmNlcyBi ZXR3ZWVuIHRoZSBtb2RlIHJlcXVlc3RlZCBieSB1c2Vyc3BhY2UgaW4gQG1vZGUgYW5kIHdoYXQK ICAgICAqIGlzIGFjdHVhbGx5IHByb2dyYW1tZWQgaW50byB0aGUgaGFyZHdhcmUuCiAgICAgKgog ICAgICogRm9yIGRyaXZlcnMgdXNpbmcgZHJtX2JyaWRnZSwgdGhpcyBzdG9yZSB0aGUgaGFyZHdh cmUgZGlzcGxheSB0aW1pbmdzCiAgICAgKiB1c2VkIGJldHdlZW4gdGhlIENSVEMgYW5kIHRoZSBm aXJzdCBicmlkZ2UuIEZvciBvdGhlciBkcml2ZXJzLCB0aGUKICAgICAqIG1lYW5pbmcgb2YgdGhl IGFkanVzdGVkX21vZGUgZmllbGQgaXMgcHVyZWx5IGRyaXZlciBpbXBsZW1lbnRhdGlvbgogICAg ICogZGVmaW5lZCBpbmZvcm1hdGlvbiwgYW5kIHdpbGwgdXN1YWxseSBiZSB1c2VkIHRvIHN0b3Jl IHRoZSBoYXJkd2FyZQogICAgICogZGlzcGxheSB0aW1pbmdzIHVzZWQgYmV0d2VlbiB0aGUgQ1JU QyBhbmQgZW5jb2RlciBibG9ja3MuCiAgICAgKi8KCi0tIApSZWdhcmRzLAoKTGF1cmVudCBQaW5j aGFydAoKCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpk cmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0 cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755479AbeDFOxQ (ORCPT ); Fri, 6 Apr 2018 10:53:16 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:35290 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755415AbeDFOxM (ORCPT ); Fri, 6 Apr 2018 10:53:12 -0400 From: Laurent Pinchart To: Philippe Cornu 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 Date: Fri, 06 Apr 2018 17:53:09 +0300 Message-ID: <1765089.gI5krnOhAB@avalon> Organization: Ideas on Board Oy In-Reply-To: <20180226121605.12050-1-philippe.cornu@st.com> References: <20180226121605.12050-1-philippe.cornu@st.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. */ -- Regards, Laurent Pinchart