[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe Cornu]

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

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe Cornu]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe Cornu]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Brian Norris]

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>

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Brian Norris]

On Thu, Jan 25, 2018 at 7:55 AM, Philippe Cornu <philippe.cornu-qxv4g6HH51o@public.gmane.org> 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-qxv4g6HH51o@public.gmane.org>
> ---
> 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Brian Norris]

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>

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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>
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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>
> 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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>
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Archit Taneja]

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@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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Archit Taneja]

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@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

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Archit Taneja]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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);
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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);
> 

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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);
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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);
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Philippe CORNU]

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);
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Laurent Pinchart]

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

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Yannick FERTRE]

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);
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Yannick FERTRE]

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);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Yannick FERTRE]

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);
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Andrzej Hajda]

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);

[PATCH] drm/bridge/synopsys: dsi: use adjusted_mode in mode_set

[Andrzej Hajda]

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);