[PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

[PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc

[Philippe Cornu]

This patch clarifies the adjusted_mode documentation
for a bridge directly connected to a crtc.

Signed-off-by: Philippe Cornu <philippe.cornu@st.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.
 	 */
 	void (*mode_set)(struct drm_bridge *bridge,
 			 struct drm_display_mode *mode,
-- 
2.15.1

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

[Daniel Vetter]

On Mon, Feb 26, 2018 at 01:16:04PM +0100, 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@st.com>
> ---
> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Aside, and kinda why I noticed this patch to begin with: You have drm-misc
commit rights, but you seem to not use that. And with 18 patches in the
4.17 cycle you're the top contributor who's not pushing his own patches.

What's the hold-up? Tools don't work, or something else? I really think
regular contributors should just push their own stuff themselves (after
appropriate review ofc).
-Daniel

> 
>  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.
>  	 */
>  	void (*mode_set)(struct drm_bridge *bridge,
>  			 struct drm_display_mode *mode,
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Philippe CORNU]

Hi Daniel,

On 03/27/2018 05:51 PM, Daniel Vetter wrote:
> On Mon, Feb 26, 2018 at 01:16:04PM +0100, 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@st.com>
>> ---
>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Many thanks for the review.

> 
> Aside, and kinda why I noticed this patch to begin with: You have drm-misc
> commit rights, but you seem to not use that. And with 18 patches in the
> 4.17 cycle you're the top contributor who's not pushing his own patches.
> 
> What's the hold-up? Tools don't work, or something else? I really think
> regular contributors should just push their own stuff themselves (after
> appropriate review ofc).
> -Daniel
> 

I still consider myself a drm “beginner”, my first patch in drm was last 
summer so less than 1 year ago.
However, thank you for your encouraging return, I will immediately study 
the matter (dim...) and prepare myself.

Thank you,
Philippe :-)

>>
>>   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.
>>   	 */
>>   	void (*mode_set)(struct drm_bridge *bridge,
>>   			 struct drm_display_mode *mode,
>> -- 
>> 2.15.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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

[Daniel Vetter]

On Thu, Mar 29, 2018 at 9:35 AM, Philippe CORNU <philippe.cornu@st.com> wrote:
> Hi Daniel,
>
> On 03/27/2018 05:51 PM, Daniel Vetter wrote:
>> On Mon, Feb 26, 2018 at 01:16:04PM +0100, 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@st.com>
>>> ---
>>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Many thanks for the review.
>
>>
>> Aside, and kinda why I noticed this patch to begin with: You have drm-misc
>> commit rights, but you seem to not use that. And with 18 patches in the
>> 4.17 cycle you're the top contributor who's not pushing his own patches.
>>
>> What's the hold-up? Tools don't work, or something else? I really think
>> regular contributors should just push their own stuff themselves (after
>> appropriate review ofc).
>> -Daniel
>>
>
> I still consider myself a drm “beginner”, my first patch in drm was last
> summer so less than 1 year ago.

You're doing great patches, and at least for drm-misc you're the top
contributor who doesn't push stuff himself. You're definitely ready to
do so!

> However, thank you for your encouraging return, I will immediately study
> the matter (dim...) and prepare myself.

Yes please. And for any questions please ask on #dri-devel on
freenode, we're all happy to help out.
-Daniel

>
> Thank you,
> Philippe :-)
>
>>>
>>>   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.
>>>       */
>>>      void (*mode_set)(struct drm_bridge *bridge,
>>>                       struct drm_display_mode *mode,
>>> --
>>> 2.15.1
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Philippe CORNU]

On 03/29/2018 09:39 AM, Daniel Vetter wrote:
> On Thu, Mar 29, 2018 at 9:35 AM, Philippe CORNU <philippe.cornu@st.com> wrote:
>> Hi Daniel,
>>
>> On 03/27/2018 05:51 PM, Daniel Vetter wrote:
>>> On Mon, Feb 26, 2018 at 01:16:04PM +0100, 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@st.com>
>>>> ---
>>>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Many thanks for the review.
>>
>>>
>>> Aside, and kinda why I noticed this patch to begin with: You have drm-misc
>>> commit rights, but you seem to not use that. And with 18 patches in the
>>> 4.17 cycle you're the top contributor who's not pushing his own patches.
>>>
>>> What's the hold-up? Tools don't work, or something else? I really think
>>> regular contributors should just push their own stuff themselves (after
>>> appropriate review ofc).
>>> -Daniel
>>>
>>
>> I still consider myself a drm “beginner”, my first patch in drm was last
>> summer so less than 1 year ago.
> 
> You're doing great patches, and at least for drm-misc you're the top
> contributor who doesn't push stuff himself. You're definitely ready to
> do so!
> 
>> However, thank you for your encouraging return, I will immediately study
>> the matter (dim...) and prepare myself.
> 
> Yes please. And for any questions please ask on #dri-devel on
> freenode, we're all happy to help out.
> -Daniel

Hi Daniel & Benjamin,

I managed to push a first patch on drm-misc-next :-)

No particular difficulties with dim installation, main encountered 
issues are "corporate proxies" & ubuntu package updates...

So this email is mostly to thank you Benjamin for your good advice and 
support when using dim.

Big thanks,
Philippe :-)

> 
>>
>> Thank you,
>> Philippe :-)
>>
>>>>
>>>>    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.
>>>>        */
>>>>       void (*mode_set)(struct drm_bridge *bridge,
>>>>                        struct drm_display_mode *mode,
>>>> --
>>>> 2.15.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 

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

[Laurent Pinchart]

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@st.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.
     */

-- 
Regards,

Laurent Pinchart

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

[Philippe CORNU]

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@st.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 :-)

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

[Daniel Vetter]

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 <philippe.cornu@st.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.

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