Re: [PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc
On Fri, Apr 06, 2018 at 03:28:27PM +, 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 ___ 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
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 :-) ___ 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
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 ___ 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
On 03/29/2018 09:39 AM, Daniel Vetter wrote: > On Thu, Mar 29, 2018 at 9:35 AM, Philippe CORNU 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 --- This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367 >>> >>> Reviewed-by: Daniel Vetter >> >> 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 > > > ___ 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
On Thu, Mar 29, 2018 at 9:35 AM, Philippe CORNU 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 >>> --- >>> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367 >> >> Reviewed-by: Daniel Vetter > > 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 ___ 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
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 >> --- >> This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367 > > Reviewed-by: Daniel Vetter 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 > ___ 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
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 > --- > This patch is linked to the discussion https://lkml.org/lkml/2018/1/25/367 Reviewed-by: Daniel Vetter 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: clarify adjusted_mode for a bridge connected to a crtc
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. */ 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