linux-kernel - Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8c52302-2e70-ab55-54c0-dfde8b42e430@st.com>
Date:   Fri, 6 Apr 2018 15:28:27 +0000
From:   Philippe CORNU <philippe.cornu@...com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC:     Daniel Vetter <daniel.vetter@...el.com>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        David Airlie <airlied@...ux.ie>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Benjamin Gaignard" <benjamin.gaignard@...aro.org>,
        Yannick FERTRE <yannick.fertre@...com>,
        Vincent ABRIOU <vincent.abriou@...com>
Subject: Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a
 crtc

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 <philippe.cornu@...com>
>> ---
>> 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 :-)