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 <[email protected]>
---
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
On Thu, Jan 25, 2018 at 7:55 AM, Philippe Cornu <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> 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
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 <[email protected]>
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
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 <[email protected]>
>
> 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);
>
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 <[email protected]>
> >
> > 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
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 <[email protected]>
>>>
>>> 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);
>
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 <[email protected]>
> >>>
> >>> 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
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 <[email protected]>
> > >>>
> > >>> 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
Hi Philippe,
I've tried your patch on both 4.17-rc1 & drm-misc-next and it works fine.
So,
Reviewed-by: Yannick Fertré <[email protected]>
Tested-by: Yannick Fertré <[email protected]>
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 <[email protected]>
>>>>>>
>>>>>> 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);
>
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 <[email protected]>
> ---
> 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);