* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set @ 2018-01-25 15:55 ` Philippe Cornu 2018-01-25 22:47 ` Brian Norris ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Philippe Cornu @ 2018-01-25 15:55 UTC (permalink / raw) To: linux-arm-kernel The "adjusted_mode" clock value (ie the real pixel clock) is more accurate than "mode" clock value (ie the panel/bridge requested clock value). It offers a better preciseness for timing computations and allows to reduce the extra dsi bandwidth in burst mode (from ~20% to ~10-12%, hw platform dependant). Signed-off-by: Philippe Cornu <philippe.cornu@st.com> --- Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel clock" drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index ed8af32f8e52..b926b62e9e33 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, clk_prepare_enable(dsi->pclk); - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, dsi->mode_flags, dsi->lanes, dsi->format, &dsi->lane_mbps); if (ret) DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); pm_runtime_get_sync(dsi->dev); dw_mipi_dsi_init(dsi); - dw_mipi_dsi_dpi_config(dsi, mode); + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); dw_mipi_dsi_packet_handler_config(dsi); dw_mipi_dsi_video_mode_config(dsi); - dw_mipi_dsi_video_packet_config(dsi, mode); + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); dw_mipi_dsi_command_mode_config(dsi); - dw_mipi_dsi_line_timer_config(dsi, mode); - dw_mipi_dsi_vertical_timing_config(dsi, mode); + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); dw_mipi_dsi_dphy_init(dsi); dw_mipi_dsi_dphy_timing_config(dsi); @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, dw_mipi_dsi_dphy_enable(dsi); - dw_mipi_dsi_wait_for_two_frames(mode); + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ dw_mipi_dsi_set_mode(dsi, 0); -- 2.15.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-01-25 15:55 ` [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set Philippe Cornu @ 2018-01-25 22:47 ` Brian Norris 2018-01-26 9:54 ` Philippe CORNU 2018-01-29 9:46 ` Laurent Pinchart 2018-04-26 6:33 ` Andrzej Hajda 2 siblings, 1 reply; 12+ messages in thread From: Brian Norris @ 2018-01-25 22:47 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jan 25, 2018 at 7:55 AM, Philippe Cornu <philippe.cornu@st.com> wrote: > The "adjusted_mode" clock value (ie the real pixel clock) is more > accurate than "mode" clock value (ie the panel/bridge requested > clock value). It offers a better preciseness for timing > computations and allows to reduce the extra dsi bandwidth in > burst mode (from ~20% to ~10-12%, hw platform dependant). > > Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > --- > Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel clock" These two appear to be the same for my cases, but at least nothing breaks: Tested-by: Brian Norris <briannorris@chromium.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-01-25 22:47 ` Brian Norris @ 2018-01-26 9:54 ` Philippe CORNU 2018-01-29 4:14 ` Archit Taneja 0 siblings, 1 reply; 12+ messages in thread From: Philippe CORNU @ 2018-01-26 9:54 UTC (permalink / raw) To: linux-arm-kernel Hi Brian, And a big thanks for your Tested-by On 01/25/2018 11:47 PM, Brian Norris wrote: > On Thu, Jan 25, 2018 at 7:55 AM, Philippe Cornu <philippe.cornu@st.com> wrote: >> The "adjusted_mode" clock value (ie the real pixel clock) is more >> accurate than "mode" clock value (ie the panel/bridge requested >> clock value). It offers a better preciseness for timing >> computations and allows to reduce the extra dsi bandwidth in >> burst mode (from ~20% to ~10-12%, hw platform dependant). >> >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> >> --- >> Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel clock" > > These two appear to be the same for my cases, but at least nothing breaks: > In drivers/gpu/drm/rockchip/rockchip_drm_vop.c function vop_crtc_mode_fixup(), the adjusted_mode->clock (ie. vop px clk output = dw dsi px clk input) is updated according to rockchip hw pll/dividers... So you "may" have a different value in adjusted_mode->clock compare to mode->clock. Maybe there is no difference for the panel you are using because its px clock matches perfectly with rockchip hw pll/dividers... or has been set to match with ;-) I did a similar patch (see [1]) and it works "fine" on stm, the only difference with the rockchip vop is that clk_round_rate() returns odd values on stm so I used set/get_rate instead. So now, both rockchip & stm crtc have an "adjusted_mode->clock" so it makes sense to use it in dw dsi :) Philippe :-) [1] https://patchwork.freedesktop.org/patch/200720/ "[PATCH] drm/stm: ltdc: use crtc_mode_fixup to update adjusted_mode clock" > Tested-by: Brian Norris <briannorris@chromium.org> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-01-26 9:54 ` Philippe CORNU @ 2018-01-29 4:14 ` Archit Taneja 0 siblings, 0 replies; 12+ messages in thread From: Archit Taneja @ 2018-01-29 4:14 UTC (permalink / raw) To: linux-arm-kernel On 01/26/2018 03:24 PM, Philippe CORNU wrote: > Hi Brian, > And a big thanks for your Tested-by > > On 01/25/2018 11:47 PM, Brian Norris wrote: >> On Thu, Jan 25, 2018 at 7:55 AM, Philippe Cornu <philippe.cornu@st.com> wrote: >>> The "adjusted_mode" clock value (ie the real pixel clock) is more >>> accurate than "mode" clock value (ie the panel/bridge requested >>> clock value). It offers a better preciseness for timing >>> computations and allows to reduce the extra dsi bandwidth in >>> burst mode (from ~20% to ~10-12%, hw platform dependant). >>> >>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> >>> --- >>> Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel clock" >> >> These two appear to be the same for my cases, but at least nothing breaks: >> > > In drivers/gpu/drm/rockchip/rockchip_drm_vop.c function > vop_crtc_mode_fixup(), the adjusted_mode->clock (ie. vop px clk output = > dw dsi px clk input) is updated according to rockchip hw pll/dividers... > > So you "may" have a different value in adjusted_mode->clock compare to > mode->clock. Maybe there is no difference for the panel you are using > because its px clock matches perfectly with rockchip hw pll/dividers... > or has been set to match with ;-) > > I did a similar patch (see [1]) and it works "fine" on stm, the only > difference with the rockchip vop is that clk_round_rate() returns odd > values on stm so I used set/get_rate instead. > > So now, both rockchip & stm crtc have an "adjusted_mode->clock" so it > makes sense to use it in dw dsi :) Could you get the patch [1] queued on drm-misc-next? I can queue this patch after it. Thanks, Archit > > Philippe :-) > > [1] https://patchwork.freedesktop.org/patch/200720/ > "[PATCH] drm/stm: ltdc: use crtc_mode_fixup to update adjusted_mode clock" > > >> Tested-by: Brian Norris <briannorris@chromium.org> >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-01-25 15:55 ` [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set Philippe Cornu 2018-01-25 22:47 ` Brian Norris @ 2018-01-29 9:46 ` Laurent Pinchart 2018-01-29 10:17 ` Philippe CORNU 2018-04-26 6:33 ` Andrzej Hajda 2 siblings, 1 reply; 12+ messages in thread From: Laurent Pinchart @ 2018-01-29 9:46 UTC (permalink / raw) To: linux-arm-kernel Hi Philippe, (CC'ing Daniel Vetter) Thank you for the patch. On Thursday, 25 January 2018 17:55:04 EET Philippe Cornu wrote: > The "adjusted_mode" clock value (ie the real pixel clock) is more > accurate than "mode" clock value (ie the panel/bridge requested > clock value). It offers a better preciseness for timing > computations and allows to reduce the extra dsi bandwidth in > burst mode (from ~20% to ~10-12%, hw platform dependant). > > Signed-off-by: Philippe Cornu <philippe.cornu@st.com> The adjusted mode is documented as /** * @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. It is purely driver * implementation defined what exactly this adjusted mode means. Usually * it is used to store the hardware display timings used between the * CRTC and encoder blocks. */ This is easy to handle when the CRTC and encoder are controlled by the same driver, as the field is "implementation defined" by a single driver . However, when using bridges, there are two drivers involved, and they must both agree to meaningfully use the adjusted mode. I can't see how to do so without standardizing the meaning of the adjusted mode field. Daniel, what's your opinion on this ? > --- > Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel > clock" > > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index > ed8af32f8e52..b926b62e9e33 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct > drm_bridge *bridge, > > clk_prepare_enable(dsi->pclk); > > - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, > + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, dsi->mode_flags, > dsi->lanes, dsi->format, &dsi->lane_mbps); > if (ret) > DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); > > pm_runtime_get_sync(dsi->dev); > dw_mipi_dsi_init(dsi); > - dw_mipi_dsi_dpi_config(dsi, mode); > + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); > dw_mipi_dsi_packet_handler_config(dsi); > dw_mipi_dsi_video_mode_config(dsi); > - dw_mipi_dsi_video_packet_config(dsi, mode); > + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); > dw_mipi_dsi_command_mode_config(dsi); > - dw_mipi_dsi_line_timer_config(dsi, mode); > - dw_mipi_dsi_vertical_timing_config(dsi, mode); > + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); > + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); > > dw_mipi_dsi_dphy_init(dsi); > dw_mipi_dsi_dphy_timing_config(dsi); > @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct > drm_bridge *bridge, > > dw_mipi_dsi_dphy_enable(dsi); > > - dw_mipi_dsi_wait_for_two_frames(mode); > + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); > > /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ > dw_mipi_dsi_set_mode(dsi, 0); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-01-29 9:46 ` Laurent Pinchart @ 2018-01-29 10:17 ` Philippe CORNU 2018-01-29 10:40 ` Laurent Pinchart 0 siblings, 1 reply; 12+ messages in thread From: Philippe CORNU @ 2018-01-29 10:17 UTC (permalink / raw) To: linux-arm-kernel Hi Laurent, And many thanks for your comments :) On 01/29/2018 10:46 AM, Laurent Pinchart wrote: > Hi Philippe, > > (CC'ing Daniel Vetter) > > Thank you for the patch. > > On Thursday, 25 January 2018 17:55:04 EET Philippe Cornu wrote: >> The "adjusted_mode" clock value (ie the real pixel clock) is more >> accurate than "mode" clock value (ie the panel/bridge requested >> clock value). It offers a better preciseness for timing >> computations and allows to reduce the extra dsi bandwidth in >> burst mode (from ~20% to ~10-12%, hw platform dependant). >> >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > > The adjusted mode is documented as > > /** > * @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. It is purely driver > * implementation defined what exactly this adjusted mode means. Usually > * it is used to store the hardware display timings used between the > * CRTC and encoder blocks. > */ > > This is easy to handle when the CRTC and encoder are controlled by the same > driver, as the field is "implementation defined" by a single driver . However, > when using bridges, there are two drivers involved, and they must both agree > to meaningfully use the adjusted mode. I can't see how to do so without > standardizing the meaning of the adjusted mode field. This is exactly the reason why my first implementation used the dsi bridge "optional pixel clock" instead of the adjusted_mode (see [1]) But after digging more into the drm source code, I think using adjusted_mode instead of the pixel clock here brings more advantages because: * adjusted_mode is an argument of bridge mode_set() probably for being used in any manner, maybe like this :) * if the bridge "user" (crtc or a master bridge drivers) does not need to modify its adjusted_mode then mode & adjusted_mode mode_set() arguments will have the same values so "no consequence" for the bridge. * if the bridge "user" (crtc or master bridge drivers) needs to adjust any value of the mode then this adjustment is available for the bridge. * rockchip crtc updates a part of the mode (the clock), stm is doing the same (see [2]) but any future "user" of the dw_mipi_dsi bridge can adjust something else (blankings...) and the dw_mipi_dsi bridge will be then aware of... But maybe it is a wrong usage of the "adjusted_mode offer"... Many thanks, Philippe :-) [1] https://patchwork.freedesktop.org/patch/200240/ [2] https://patchwork.freedesktop.org/patch/200720/ > > Daniel, what's your opinion on this ? > >> --- >> Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel >> clock" >> >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index >> ed8af32f8e52..b926b62e9e33 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct >> drm_bridge *bridge, >> >> clk_prepare_enable(dsi->pclk); >> >> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, >> + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, dsi->mode_flags, >> dsi->lanes, dsi->format, &dsi->lane_mbps); >> if (ret) >> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); >> >> pm_runtime_get_sync(dsi->dev); >> dw_mipi_dsi_init(dsi); >> - dw_mipi_dsi_dpi_config(dsi, mode); >> + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); >> dw_mipi_dsi_packet_handler_config(dsi); >> dw_mipi_dsi_video_mode_config(dsi); >> - dw_mipi_dsi_video_packet_config(dsi, mode); >> + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); >> dw_mipi_dsi_command_mode_config(dsi); >> - dw_mipi_dsi_line_timer_config(dsi, mode); >> - dw_mipi_dsi_vertical_timing_config(dsi, mode); >> + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); >> + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); >> >> dw_mipi_dsi_dphy_init(dsi); >> dw_mipi_dsi_dphy_timing_config(dsi); >> @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct >> drm_bridge *bridge, >> >> dw_mipi_dsi_dphy_enable(dsi); >> >> - dw_mipi_dsi_wait_for_two_frames(mode); >> + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); >> >> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ >> dw_mipi_dsi_set_mode(dsi, 0); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-01-29 10:17 ` Philippe CORNU @ 2018-01-29 10:40 ` Laurent Pinchart 2018-02-02 22:41 ` Philippe CORNU 0 siblings, 1 reply; 12+ messages in thread From: Laurent Pinchart @ 2018-01-29 10:40 UTC (permalink / raw) To: linux-arm-kernel Hi Philippe, On Monday, 29 January 2018 12:17:37 EET Philippe CORNU wrote: > On 01/29/2018 10:46 AM, Laurent Pinchart wrote: > > On Thursday, 25 January 2018 17:55:04 EET Philippe Cornu wrote: > > > >> The "adjusted_mode" clock value (ie the real pixel clock) is more > >> accurate than "mode" clock value (ie the panel/bridge requested > >> clock value). It offers a better preciseness for timing > >> computations and allows to reduce the extra dsi bandwidth in > >> burst mode (from ~20% to ~10-12%, hw platform dependant). > >> > >> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > > > > The adjusted mode is documented as > > > > /** > > * @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. It is purely driver > > * implementation defined what exactly this adjusted mode means. Usually > > * it is used to store the hardware display timings used between the > > * CRTC and encoder blocks. > > */ > > > > This is easy to handle when the CRTC and encoder are controlled by the > > same driver, as the field is "implementation defined" by a single driver > > . However, when using bridges, there are two drivers involved, and they > > must both agree to meaningfully use the adjusted mode. I can't see how to > > do so without standardizing the meaning of the adjusted mode field. > > This is exactly the reason why my first implementation used the dsi > bridge "optional pixel clock" instead of the adjusted_mode (see [1]) > > But after digging more into the drm source code, I think using > adjusted_mode instead of the pixel clock here brings more advantages > because: > * adjusted_mode is an argument of bridge mode_set() probably for being > used in any manner, maybe like this :) > * if the bridge "user" (crtc or a master bridge drivers) does not need > to modify its adjusted_mode then mode & adjusted_mode mode_set() > arguments will have the same values so "no consequence" for the bridge. > * if the bridge "user" (crtc or master bridge drivers) needs to adjust > any value of the mode then this adjustment is available for the bridge. Remember that there can be multiple chained bridges, and a single adjusted mode field. > * rockchip crtc updates a part of the mode (the clock), stm is doing the > same (see [2]) but any future "user" of the dw_mipi_dsi bridge can > adjust something else (blankings...) and the dw_mipi_dsi bridge will be > then aware of... > > But maybe it is a wrong usage of the "adjusted_mode offer"... I don't disagree that there's a need for using adjusted values, but I believe we need to create a clear API to do so. Using the adjust_mode field as-is when it's clearly documented as being implementation-defined is asking for trouble. > Many thanks, > Philippe :-) > [1] https://patchwork.freedesktop.org/patch/200240/ > [2] https://patchwork.freedesktop.org/patch/200720/ > > > Daniel, what's your opinion on this ? > > > >> --- > >> Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel > >> clock" > >> > >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index > >> ed8af32f8e52..b926b62e9e33 100644 > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct > >> drm_bridge *bridge, > >> > >> clk_prepare_enable(dsi->pclk); > >> > >> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, > >> + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, > >> dsi->mode_flags, > >> dsi->lanes, dsi->format, &dsi->lane_mbps); > >> if (ret) > >> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); > >> > >> pm_runtime_get_sync(dsi->dev); > >> dw_mipi_dsi_init(dsi); > >> > >> - dw_mipi_dsi_dpi_config(dsi, mode); > >> + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); > >> dw_mipi_dsi_packet_handler_config(dsi); > >> dw_mipi_dsi_video_mode_config(dsi); > >> - dw_mipi_dsi_video_packet_config(dsi, mode); > >> + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); > >> dw_mipi_dsi_command_mode_config(dsi); > >> - dw_mipi_dsi_line_timer_config(dsi, mode); > >> - dw_mipi_dsi_vertical_timing_config(dsi, mode); > >> + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); > >> + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); > >> dw_mipi_dsi_dphy_init(dsi); > >> dw_mipi_dsi_dphy_timing_config(dsi); > >> > >> @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct > >> drm_bridge *bridge, > >> > >> dw_mipi_dsi_dphy_enable(dsi); > >> > >> - dw_mipi_dsi_wait_for_two_frames(mode); > >> + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); > >> > >> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ > >> dw_mipi_dsi_set_mode(dsi, 0); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-01-29 10:40 ` Laurent Pinchart @ 2018-02-02 22:41 ` Philippe CORNU 2018-02-08 13:15 ` Laurent Pinchart 0 siblings, 1 reply; 12+ messages in thread From: Philippe CORNU @ 2018-02-02 22:41 UTC (permalink / raw) To: linux-arm-kernel Hi Laurent & Daniel :-) On 01/29/2018 11:40 AM, Laurent Pinchart wrote: > Hi Philippe, > > On Monday, 29 January 2018 12:17:37 EET Philippe CORNU wrote: >> On 01/29/2018 10:46 AM, Laurent Pinchart wrote: >>> On Thursday, 25 January 2018 17:55:04 EET Philippe Cornu wrote: >>> >>>> The "adjusted_mode" clock value (ie the real pixel clock) is more >>>> accurate than "mode" clock value (ie the panel/bridge requested >>>> clock value). It offers a better preciseness for timing >>>> computations and allows to reduce the extra dsi bandwidth in >>>> burst mode (from ~20% to ~10-12%, hw platform dependant). >>>> >>>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> >>> >>> The adjusted mode is documented as >>> >>> /** >>> * @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. It is purely driver >>> * implementation defined what exactly this adjusted mode means. Usually >>> * it is used to store the hardware display timings used between the >>> * CRTC and encoder blocks. >>> */ >>> >>> This is easy to handle when the CRTC and encoder are controlled by the >>> same driver, as the field is "implementation defined" by a single driver >>> . However, when using bridges, there are two drivers involved, and they >>> must both agree to meaningfully use the adjusted mode. I can't see how to >>> do so without standardizing the meaning of the adjusted mode field. >> >> This is exactly the reason why my first implementation used the dsi >> bridge "optional pixel clock" instead of the adjusted_mode (see [1]) >> >> But after digging more into the drm source code, I think using >> adjusted_mode instead of the pixel clock here brings more advantages >> because: >> * adjusted_mode is an argument of bridge mode_set() probably for being >> used in any manner, maybe like this :) >> * if the bridge "user" (crtc or a master bridge drivers) does not need >> to modify its adjusted_mode then mode & adjusted_mode mode_set() >> arguments will have the same values so "no consequence" for the bridge. >> * if the bridge "user" (crtc or master bridge drivers) needs to adjust >> any value of the mode then this adjustment is available for the bridge. > > Remember that there can be multiple chained bridges, and a single adjusted > mode field. > >> * rockchip crtc updates a part of the mode (the clock), stm is doing the >> same (see [2]) but any future "user" of the dw_mipi_dsi bridge can >> adjust something else (blankings...) and the dw_mipi_dsi bridge will be >> then aware of... >> >> But maybe it is a wrong usage of the "adjusted_mode offer"... > > I don't disagree that there's a need for using adjusted values, but I believe > we need to create a clear API to do so. Using the adjust_mode field as-is when > it's clearly documented as being implementation-defined is asking for trouble. > Laurent, do you think we can use "adjusted mode" here in this small patch as the actual 2 "users" of this bridge (rockchip & stm) use both "adjusted mode" in their crtc? >> Many thanks, >> Philippe :-) >> [1] https://patchwork.freedesktop.org/patch/200240/ >> [2] https://patchwork.freedesktop.org/patch/200720/ >> >>> Daniel, what's your opinion on this ? >>> Daniel, any opinion on the adjusted_mode usage? Many thanks to both of you, Philippe :-) >>>> --- >>>> Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel >>>> clock" >>>> >>>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ >>>> 1 file changed, 6 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index >>>> ed8af32f8e52..b926b62e9e33 100644 >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>> @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct >>>> drm_bridge *bridge, >>>> >>>> clk_prepare_enable(dsi->pclk); >>>> >>>> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, >>>> + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, >>>> dsi->mode_flags, >>>> dsi->lanes, dsi->format, &dsi->lane_mbps); >>>> if (ret) >>>> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); >>>> >>>> pm_runtime_get_sync(dsi->dev); >>>> dw_mipi_dsi_init(dsi); >>>> >>>> - dw_mipi_dsi_dpi_config(dsi, mode); >>>> + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); >>>> dw_mipi_dsi_packet_handler_config(dsi); >>>> dw_mipi_dsi_video_mode_config(dsi); >>>> - dw_mipi_dsi_video_packet_config(dsi, mode); >>>> + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); >>>> dw_mipi_dsi_command_mode_config(dsi); >>>> - dw_mipi_dsi_line_timer_config(dsi, mode); >>>> - dw_mipi_dsi_vertical_timing_config(dsi, mode); >>>> + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); >>>> + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); >>>> dw_mipi_dsi_dphy_init(dsi); >>>> dw_mipi_dsi_dphy_timing_config(dsi); >>>> >>>> @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct >>>> drm_bridge *bridge, >>>> >>>> dw_mipi_dsi_dphy_enable(dsi); >>>> >>>> - dw_mipi_dsi_wait_for_two_frames(mode); >>>> + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); >>>> >>>> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ >>>> dw_mipi_dsi_set_mode(dsi, 0); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-02-02 22:41 ` Philippe CORNU @ 2018-02-08 13:15 ` Laurent Pinchart 2018-04-23 10:10 ` Laurent Pinchart 0 siblings, 1 reply; 12+ messages in thread From: Laurent Pinchart @ 2018-02-08 13:15 UTC (permalink / raw) To: linux-arm-kernel Hi Philippe, On Saturday, 3 February 2018 00:41:26 EET Philippe CORNU wrote: > On 01/29/2018 11:40 AM, Laurent Pinchart wrote: > > On Monday, 29 January 2018 12:17:37 EET Philippe CORNU wrote: > >> On 01/29/2018 10:46 AM, Laurent Pinchart wrote: > >>> On Thursday, 25 January 2018 17:55:04 EET Philippe Cornu wrote: > >>>> The "adjusted_mode" clock value (ie the real pixel clock) is more > >>>> accurate than "mode" clock value (ie the panel/bridge requested > >>>> clock value). It offers a better preciseness for timing > >>>> computations and allows to reduce the extra dsi bandwidth in > >>>> burst mode (from ~20% to ~10-12%, hw platform dependant). > >>>> > >>>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > >>> > >>> The adjusted mode is documented as > >>> > >>> /** > >>> * @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. It is purely driver > >>> * implementation defined what exactly this adjusted mode means. > >>> Usually > >>> * it is used to store the hardware display timings used between the > >>> * CRTC and encoder blocks. > >>> */ > >>> > >>> This is easy to handle when the CRTC and encoder are controlled by the > >>> same driver, as the field is "implementation defined" by a single > >>> driver. However, when using bridges, there are two drivers involved, and > >>> they must both agree to meaningfully use the adjusted mode. I can't see > >>> how to do so without standardizing the meaning of the adjusted mode > >>> field. > >> > >> This is exactly the reason why my first implementation used the dsi > >> bridge "optional pixel clock" instead of the adjusted_mode (see [1]) > >> > >> But after digging more into the drm source code, I think using > >> adjusted_mode instead of the pixel clock here brings more advantages > >> because: > >> * adjusted_mode is an argument of bridge mode_set() probably for being > >> used in any manner, maybe like this :) > >> * if the bridge "user" (crtc or a master bridge drivers) does not need > >> to modify its adjusted_mode then mode & adjusted_mode mode_set() > >> arguments will have the same values so "no consequence" for the bridge. > >> * if the bridge "user" (crtc or master bridge drivers) needs to adjust > >> any value of the mode then this adjustment is available for the bridge. > > > > Remember that there can be multiple chained bridges, and a single > > adjusted mode field. > > > >> * rockchip crtc updates a part of the mode (the clock), stm is doing the > >> same (see [2]) but any future "user" of the dw_mipi_dsi bridge can > >> adjust something else (blankings...) and the dw_mipi_dsi bridge will be > >> then aware of... > >> > >> But maybe it is a wrong usage of the "adjusted_mode offer"... > > > > I don't disagree that there's a need for using adjusted values, but I > > believe we need to create a clear API to do so. Using the adjust_mode > > field as-is when it's clearly documented as being implementation-defined > > is asking for trouble. > > Laurent, do you think we can use "adjusted mode" here in this small > patch as the actual 2 "users" of this bridge (rockchip & stm) use both > "adjusted mode" in their crtc? The Synopsys DSI driver might only be used by two display controller drivers today, it is nonetheless a standard bridge driver that should not make any assumption of a particular use of the adjusted_mode in particular display controller drivers. To use the adjusted_mode value in bridge drivers we need to standardize its usage, otherwise we'll end up with incompatibilities between bridge drivers and display controller drivers. > >> [1] https://patchwork.freedesktop.org/patch/200240/ > >> [2] https://patchwork.freedesktop.org/patch/200720/ > >> > >>> Daniel, what's your opinion on this ? > > Daniel, any opinion on the adjusted_mode usage? > > >>>> --- > >>>> Note: This patch replaces "drm/bridge/synopsys: dsi: add optional > >>>> pixel clock" > >>>> > >>>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ > >>>> 1 file changed, 6 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index > >>>> ed8af32f8e52..b926b62e9e33 100644 > >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>>> @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct > >>>> drm_bridge *bridge, > >>>> > >>>> clk_prepare_enable(dsi->pclk); > >>>> > >>>> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, > >>>> + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, > >>>> dsi->mode_flags, > >>>> dsi->lanes, dsi->format, &dsi->lane_mbps); > >>>> if (ret) > >>>> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); > >>>> > >>>> pm_runtime_get_sync(dsi->dev); > >>>> dw_mipi_dsi_init(dsi); > >>>> > >>>> - dw_mipi_dsi_dpi_config(dsi, mode); > >>>> + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); > >>>> dw_mipi_dsi_packet_handler_config(dsi); > >>>> dw_mipi_dsi_video_mode_config(dsi); > >>>> - dw_mipi_dsi_video_packet_config(dsi, mode); > >>>> + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); > >>>> dw_mipi_dsi_command_mode_config(dsi); > >>>> - dw_mipi_dsi_line_timer_config(dsi, mode); > >>>> - dw_mipi_dsi_vertical_timing_config(dsi, mode); > >>>> + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); > >>>> + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); > >>>> dw_mipi_dsi_dphy_init(dsi); > >>>> dw_mipi_dsi_dphy_timing_config(dsi); > >>>> > >>>> @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct > >>>> drm_bridge *bridge, > >>>> > >>>> dw_mipi_dsi_dphy_enable(dsi); > >>>> > >>>> - dw_mipi_dsi_wait_for_two_frames(mode); > >>>> + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); > >>>> > >>>> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare > >>>> */ > >>>> dw_mipi_dsi_set_mode(dsi, 0); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-02-08 13:15 ` Laurent Pinchart @ 2018-04-23 10:10 ` Laurent Pinchart 2018-04-23 12:29 ` Yannick FERTRE 0 siblings, 1 reply; 12+ messages in thread From: Laurent Pinchart @ 2018-04-23 10:10 UTC (permalink / raw) To: linux-arm-kernel Hi Philippe, On Thursday, 8 February 2018 15:15:40 EEST Laurent Pinchart wrote: > On Saturday, 3 February 2018 00:41:26 EET Philippe CORNU wrote: > > On 01/29/2018 11:40 AM, Laurent Pinchart wrote: > > > On Monday, 29 January 2018 12:17:37 EET Philippe CORNU wrote: > > >> On 01/29/2018 10:46 AM, Laurent Pinchart wrote: > > >>> On Thursday, 25 January 2018 17:55:04 EET Philippe Cornu wrote: > > >>>> The "adjusted_mode" clock value (ie the real pixel clock) is more > > >>>> accurate than "mode" clock value (ie the panel/bridge requested > > >>>> clock value). It offers a better preciseness for timing > > >>>> computations and allows to reduce the extra dsi bandwidth in > > >>>> burst mode (from ~20% to ~10-12%, hw platform dependant). > > >>>> > > >>>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > > >>> > > >>> The adjusted mode is documented as > > >>> > > >>> /** > > >>> > > >>> * @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. It is purely driver > > >>> * implementation defined what exactly this adjusted mode means. > > >>> Usually > > >>> * it is used to store the hardware display timings used between the > > >>> * CRTC and encoder blocks. > > >>> */ > > >>> > > >>> This is easy to handle when the CRTC and encoder are controlled by the > > >>> same driver, as the field is "implementation defined" by a single > > >>> driver. However, when using bridges, there are two drivers involved, > > >>> and > > >>> they must both agree to meaningfully use the adjusted mode. I can't > > >>> see > > >>> how to do so without standardizing the meaning of the adjusted mode > > >>> field. > > >> > > >> This is exactly the reason why my first implementation used the dsi > > >> bridge "optional pixel clock" instead of the adjusted_mode (see [1]) > > >> > > >> But after digging more into the drm source code, I think using > > >> adjusted_mode instead of the pixel clock here brings more advantages > > >> because: > > >> * adjusted_mode is an argument of bridge mode_set() probably for being > > >> used in any manner, maybe like this :) > > >> * if the bridge "user" (crtc or a master bridge drivers) does not need > > >> to modify its adjusted_mode then mode & adjusted_mode mode_set() > > >> arguments will have the same values so "no consequence" for the bridge. > > >> * if the bridge "user" (crtc or master bridge drivers) needs to adjust > > >> any value of the mode then this adjustment is available for the bridge. > > > > > > Remember that there can be multiple chained bridges, and a single > > > adjusted mode field. > > > > > >> * rockchip crtc updates a part of the mode (the clock), stm is doing > > >> the > > >> same (see [2]) but any future "user" of the dw_mipi_dsi bridge can > > >> adjust something else (blankings...) and the dw_mipi_dsi bridge will be > > >> then aware of... > > >> > > >> But maybe it is a wrong usage of the "adjusted_mode offer"... > > > > > > I don't disagree that there's a need for using adjusted values, but I > > > believe we need to create a clear API to do so. Using the adjust_mode > > > field as-is when it's clearly documented as being implementation-defined > > > is asking for trouble. > > > > Laurent, do you think we can use "adjusted mode" here in this small > > patch as the actual 2 "users" of this bridge (rockchip & stm) use both > > "adjusted mode" in their crtc? > > The Synopsys DSI driver might only be used by two display controller drivers > today, it is nonetheless a standard bridge driver that should not make any > assumption of a particular use of the adjusted_mode in particular display > controller drivers. To use the adjusted_mode value in bridge drivers we > need to standardize its usage, otherwise we'll end up with > incompatibilities between bridge drivers and display controller drivers. Now that the documentation has been clarified I have no objection against this patch anymore. > > >> [1] https://patchwork.freedesktop.org/patch/200240/ > > >> [2] https://patchwork.freedesktop.org/patch/200720/ > > >> > > >>> Daniel, what's your opinion on this ? > > > > Daniel, any opinion on the adjusted_mode usage? > > > > >>>> --- > > >>>> Note: This patch replaces "drm/bridge/synopsys: dsi: add optional > > >>>> pixel clock" > > >>>> > > >>>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ > > >>>> 1 file changed, 6 insertions(+), 6 deletions(-) > > >>>> > > >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > > >>>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index > > >>>> ed8af32f8e52..b926b62e9e33 100644 > > >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > > >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > > >>>> @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct > > >>>> drm_bridge *bridge, > > >>>> > > >>>> clk_prepare_enable(dsi->pclk); > > >>>> > > >>>> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, > > >>>> + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, > > >>>> dsi->mode_flags, > > >>>> > > >>>> dsi->lanes, dsi->format, &dsi->lane_mbps); > > >>>> > > >>>> if (ret) > > >>>> > > >>>> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); > > >>>> > > >>>> pm_runtime_get_sync(dsi->dev); > > >>>> dw_mipi_dsi_init(dsi); > > >>>> > > >>>> - dw_mipi_dsi_dpi_config(dsi, mode); > > >>>> + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); > > >>>> > > >>>> dw_mipi_dsi_packet_handler_config(dsi); > > >>>> dw_mipi_dsi_video_mode_config(dsi); > > >>>> > > >>>> - dw_mipi_dsi_video_packet_config(dsi, mode); > > >>>> + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); > > >>>> > > >>>> dw_mipi_dsi_command_mode_config(dsi); > > >>>> > > >>>> - dw_mipi_dsi_line_timer_config(dsi, mode); > > >>>> - dw_mipi_dsi_vertical_timing_config(dsi, mode); > > >>>> + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); > > >>>> + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); > > >>>> > > >>>> dw_mipi_dsi_dphy_init(dsi); > > >>>> dw_mipi_dsi_dphy_timing_config(dsi); > > >>>> > > >>>> @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct > > >>>> drm_bridge *bridge, > > >>>> > > >>>> dw_mipi_dsi_dphy_enable(dsi); > > >>>> > > >>>> - dw_mipi_dsi_wait_for_two_frames(mode); > > >>>> + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); > > >>>> > > >>>> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare > > >>>> */ > > >>>> dw_mipi_dsi_set_mode(dsi, 0); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-04-23 10:10 ` Laurent Pinchart @ 2018-04-23 12:29 ` Yannick FERTRE 0 siblings, 0 replies; 12+ messages in thread From: Yannick FERTRE @ 2018-04-23 12:29 UTC (permalink / raw) To: linux-arm-kernel Hi Philippe, I've tried your patch on both 4.17-rc1 & drm-misc-next and it works fine. So, Reviewed-by: Yannick Fertr? <yannick.fertre@st.com> Tested-by: Yannick Fertr? <yannick.fertre@st.com> BR Yannick Fertr? On 04/23/2018 12:10 PM, Laurent Pinchart wrote: > Hi Philippe, > > On Thursday, 8 February 2018 15:15:40 EEST Laurent Pinchart wrote: >> On Saturday, 3 February 2018 00:41:26 EET Philippe CORNU wrote: >>> On 01/29/2018 11:40 AM, Laurent Pinchart wrote: >>>> On Monday, 29 January 2018 12:17:37 EET Philippe CORNU wrote: >>>>> On 01/29/2018 10:46 AM, Laurent Pinchart wrote: >>>>>> On Thursday, 25 January 2018 17:55:04 EET Philippe Cornu wrote: >>>>>>> The "adjusted_mode" clock value (ie the real pixel clock) is more >>>>>>> accurate than "mode" clock value (ie the panel/bridge requested >>>>>>> clock value). It offers a better preciseness for timing >>>>>>> computations and allows to reduce the extra dsi bandwidth in >>>>>>> burst mode (from ~20% to ~10-12%, hw platform dependant). >>>>>>> >>>>>>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com> >>>>>> >>>>>> The adjusted mode is documented as >>>>>> >>>>>> /** >>>>>> >>>>>> * @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. It is purely driver >>>>>> * implementation defined what exactly this adjusted mode means. >>>>>> Usually >>>>>> * it is used to store the hardware display timings used between the >>>>>> * CRTC and encoder blocks. >>>>>> */ >>>>>> >>>>>> This is easy to handle when the CRTC and encoder are controlled by the >>>>>> same driver, as the field is "implementation defined" by a single >>>>>> driver. However, when using bridges, there are two drivers involved, >>>>>> and >>>>>> they must both agree to meaningfully use the adjusted mode. I can't >>>>>> see >>>>>> how to do so without standardizing the meaning of the adjusted mode >>>>>> field. >>>>> >>>>> This is exactly the reason why my first implementation used the dsi >>>>> bridge "optional pixel clock" instead of the adjusted_mode (see [1]) >>>>> >>>>> But after digging more into the drm source code, I think using >>>>> adjusted_mode instead of the pixel clock here brings more advantages >>>>> because: >>>>> * adjusted_mode is an argument of bridge mode_set() probably for being >>>>> used in any manner, maybe like this :) >>>>> * if the bridge "user" (crtc or a master bridge drivers) does not need >>>>> to modify its adjusted_mode then mode & adjusted_mode mode_set() >>>>> arguments will have the same values so "no consequence" for the bridge. >>>>> * if the bridge "user" (crtc or master bridge drivers) needs to adjust >>>>> any value of the mode then this adjustment is available for the bridge. >>>> >>>> Remember that there can be multiple chained bridges, and a single >>>> adjusted mode field. >>>> >>>>> * rockchip crtc updates a part of the mode (the clock), stm is doing >>>>> the >>>>> same (see [2]) but any future "user" of the dw_mipi_dsi bridge can >>>>> adjust something else (blankings...) and the dw_mipi_dsi bridge will be >>>>> then aware of... >>>>> >>>>> But maybe it is a wrong usage of the "adjusted_mode offer"... >>>> >>>> I don't disagree that there's a need for using adjusted values, but I >>>> believe we need to create a clear API to do so. Using the adjust_mode >>>> field as-is when it's clearly documented as being implementation-defined >>>> is asking for trouble. >>> >>> Laurent, do you think we can use "adjusted mode" here in this small >>> patch as the actual 2 "users" of this bridge (rockchip & stm) use both >>> "adjusted mode" in their crtc? >> >> The Synopsys DSI driver might only be used by two display controller drivers >> today, it is nonetheless a standard bridge driver that should not make any >> assumption of a particular use of the adjusted_mode in particular display >> controller drivers. To use the adjusted_mode value in bridge drivers we >> need to standardize its usage, otherwise we'll end up with >> incompatibilities between bridge drivers and display controller drivers. > > Now that the documentation has been clarified I have no objection against this > patch anymore. > >>>>> [1] https://patchwork.freedesktop.org/patch/200240/ >>>>> [2] https://patchwork.freedesktop.org/patch/200720/ >>>>> >>>>>> Daniel, what's your opinion on this ? >>> >>> Daniel, any opinion on the adjusted_mode usage? >>> >>>>>>> --- >>>>>>> Note: This patch replaces "drm/bridge/synopsys: dsi: add optional >>>>>>> pixel clock" >>>>>>> >>>>>>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ >>>>>>> 1 file changed, 6 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index >>>>>>> ed8af32f8e52..b926b62e9e33 100644 >>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>>>>> @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct >>>>>>> drm_bridge *bridge, >>>>>>> >>>>>>> clk_prepare_enable(dsi->pclk); >>>>>>> >>>>>>> - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, >>>>>>> + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, >>>>>>> dsi->mode_flags, >>>>>>> >>>>>>> dsi->lanes, dsi->format, &dsi->lane_mbps); >>>>>>> >>>>>>> if (ret) >>>>>>> >>>>>>> DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); >>>>>>> >>>>>>> pm_runtime_get_sync(dsi->dev); >>>>>>> dw_mipi_dsi_init(dsi); >>>>>>> >>>>>>> - dw_mipi_dsi_dpi_config(dsi, mode); >>>>>>> + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); >>>>>>> >>>>>>> dw_mipi_dsi_packet_handler_config(dsi); >>>>>>> dw_mipi_dsi_video_mode_config(dsi); >>>>>>> >>>>>>> - dw_mipi_dsi_video_packet_config(dsi, mode); >>>>>>> + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); >>>>>>> >>>>>>> dw_mipi_dsi_command_mode_config(dsi); >>>>>>> >>>>>>> - dw_mipi_dsi_line_timer_config(dsi, mode); >>>>>>> - dw_mipi_dsi_vertical_timing_config(dsi, mode); >>>>>>> + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); >>>>>>> + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); >>>>>>> >>>>>>> dw_mipi_dsi_dphy_init(dsi); >>>>>>> dw_mipi_dsi_dphy_timing_config(dsi); >>>>>>> >>>>>>> @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct >>>>>>> drm_bridge *bridge, >>>>>>> >>>>>>> dw_mipi_dsi_dphy_enable(dsi); >>>>>>> >>>>>>> - dw_mipi_dsi_wait_for_two_frames(mode); >>>>>>> + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); >>>>>>> >>>>>>> /* Switch to cmd mode for panel-bridge pre_enable & panel prepare >>>>>>> */ >>>>>>> dw_mipi_dsi_set_mode(dsi, 0); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set 2018-01-25 15:55 ` [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set Philippe Cornu 2018-01-25 22:47 ` Brian Norris 2018-01-29 9:46 ` Laurent Pinchart @ 2018-04-26 6:33 ` Andrzej Hajda 2 siblings, 0 replies; 12+ messages in thread From: Andrzej Hajda @ 2018-04-26 6:33 UTC (permalink / raw) To: linux-arm-kernel On 25.01.2018 16:55, Philippe Cornu wrote: > The "adjusted_mode" clock value (ie the real pixel clock) is more > accurate than "mode" clock value (ie the panel/bridge requested > clock value). It offers a better preciseness for timing > computations and allows to reduce the extra dsi bandwidth in > burst mode (from ~20% to ~10-12%, hw platform dependant). > > Signed-off-by: Philippe Cornu <philippe.cornu@st.com> > --- > Note: This patch replaces "drm/bridge/synopsys: dsi: add optional pixel clock" Queued to drm-misc-next. -- Regards Andrzej > > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index ed8af32f8e52..b926b62e9e33 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -707,20 +707,20 @@ static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, > > clk_prepare_enable(dsi->pclk); > > - ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags, > + ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, dsi->mode_flags, > dsi->lanes, dsi->format, &dsi->lane_mbps); > if (ret) > DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n"); > > pm_runtime_get_sync(dsi->dev); > dw_mipi_dsi_init(dsi); > - dw_mipi_dsi_dpi_config(dsi, mode); > + dw_mipi_dsi_dpi_config(dsi, adjusted_mode); > dw_mipi_dsi_packet_handler_config(dsi); > dw_mipi_dsi_video_mode_config(dsi); > - dw_mipi_dsi_video_packet_config(dsi, mode); > + dw_mipi_dsi_video_packet_config(dsi, adjusted_mode); > dw_mipi_dsi_command_mode_config(dsi); > - dw_mipi_dsi_line_timer_config(dsi, mode); > - dw_mipi_dsi_vertical_timing_config(dsi, mode); > + dw_mipi_dsi_line_timer_config(dsi, adjusted_mode); > + dw_mipi_dsi_vertical_timing_config(dsi, adjusted_mode); > > dw_mipi_dsi_dphy_init(dsi); > dw_mipi_dsi_dphy_timing_config(dsi); > @@ -734,7 +734,7 @@ static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge, > > dw_mipi_dsi_dphy_enable(dsi); > > - dw_mipi_dsi_wait_for_two_frames(mode); > + dw_mipi_dsi_wait_for_two_frames(adjusted_mode); > > /* Switch to cmd mode for panel-bridge pre_enable & panel prepare */ > dw_mipi_dsi_set_mode(dsi, 0); ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-04-26 6:33 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20180125155556epcas2p107973eb77ab08c436dbddcb66fcb7c89@epcas2p1.samsung.com>
2018-01-25 15:55 ` [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set Philippe Cornu
2018-01-25 22:47 ` Brian Norris
2018-01-26 9:54 ` Philippe CORNU
2018-01-29 4:14 ` Archit Taneja
2018-01-29 9:46 ` Laurent Pinchart
2018-01-29 10:17 ` Philippe CORNU
2018-01-29 10:40 ` Laurent Pinchart
2018-02-02 22:41 ` Philippe CORNU
2018-02-08 13:15 ` Laurent Pinchart
2018-04-23 10:10 ` Laurent Pinchart
2018-04-23 12:29 ` Yannick FERTRE
2018-04-26 6:33 ` Andrzej Hajda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).