* [PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate
@ 2017-12-26 11:12 Jonathan Liu
2017-12-26 11:12 ` [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate Jonathan Liu
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Jonathan Liu @ 2017-12-26 11:12 UTC (permalink / raw)
To: linux-arm-kernel
This patchset fixes several issues in sun4i_tmds_determine_rate that I
discovered while trying to get a projector connected to an Olimex
A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and
pixel clock of 83.5 MHz to display at its native resolution.
Changes for v2:
- Split into separate patches for each issue
- Add details to commit message for reproducing issue
Jonathan Liu (3):
drm/sun4i: hdmi: Check for unset best_parent in
sun4i_tmds_determine_rate
drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate
drm/sun4i: hdmi: Add missing rate halving check in
sun4i_tmds_determine_rate
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
--
2.15.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate 2017-12-26 11:12 [PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate Jonathan Liu @ 2017-12-26 11:12 ` Jonathan Liu 2018-01-04 19:56 ` Maxime Ripard 2017-12-26 11:12 ` [PATCH v2 2/3] drm/sun4i: hdmi: Fix incorrect assignment " Jonathan Liu 2017-12-26 11:12 ` [PATCH v2 3/3] drm/sun4i: hdmi: Add missing rate halving check " Jonathan Liu 2 siblings, 1 reply; 8+ messages in thread From: Jonathan Liu @ 2017-12-26 11:12 UTC (permalink / raw) To: linux-arm-kernel We should check if the best match has been set before comparing it. Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu <net147@gmail.com> --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..4d235e5ea31c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, goto out; } - if (abs(rate - rounded / i) < + if (!best_parent || abs(rate - rounded / i) < abs(rate - best_parent / best_div)) { best_parent = rounded; best_div = i; -- 2.15.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate 2017-12-26 11:12 ` [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate Jonathan Liu @ 2018-01-04 19:56 ` Maxime Ripard 2018-01-04 22:44 ` Jonathan Liu 0 siblings, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2018-01-04 19:56 UTC (permalink / raw) To: linux-arm-kernel Hi, On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote: > We should check if the best match has been set before comparing it. > > Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") > Signed-off-by: Jonathan Liu <net147@gmail.com> > --- > drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c > index dc332ea56f6c..4d235e5ea31c 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c > @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, > goto out; > } > > - if (abs(rate - rounded / i) < > + if (!best_parent || abs(rate - rounded / i) < Why is that causing any issue? If best_parent is set to 0... > abs(rate - best_parent / best_div)) { ... the value returned here is going to be rate, which is going to be higher than the first part of the comparison meaning ... > best_parent = rounded; ... that best_parent is going to be set there. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180104/5d9007ba/attachment.sig> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate 2018-01-04 19:56 ` Maxime Ripard @ 2018-01-04 22:44 ` Jonathan Liu 2018-01-05 10:03 ` Maxime Ripard 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Liu @ 2018-01-04 22:44 UTC (permalink / raw) To: linux-arm-kernel Hi Maxime, On 5 January 2018 at 06:56, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote: >> We should check if the best match has been set before comparing it. >> >> Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") >> Signed-off-by: Jonathan Liu <net147@gmail.com> >> --- >> drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> index dc332ea56f6c..4d235e5ea31c 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, >> goto out; >> } >> >> - if (abs(rate - rounded / i) < >> + if (!best_parent || abs(rate - rounded / i) < > > Why is that causing any issue? > > If best_parent is set to 0... > >> abs(rate - best_parent / best_div)) { > > ... the value returned here is going to be rate, which is going to be > higher than the first part of the comparison meaning ... > >> best_parent = rounded; > > ... that best_parent is going to be set there. Consider the following: rate = 83500000 rounded = ideal * 2 It is possible that if "rounded = clk_hw_round_rate(parent, ideal)" gives high enough values that the condition "abs(rate - rounded / i) < abs(rate - best_parent / best_div)" is never met. Then you can end up with: req->rate = 0 req->best_parent_rate = 0 req->best_parent_hw = ... Also, the sun4i_tmds_calc_divider function has a similar check. Regards, Jonathan ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate 2018-01-04 22:44 ` Jonathan Liu @ 2018-01-05 10:03 ` Maxime Ripard 2018-01-05 11:08 ` Jonathan Liu 0 siblings, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2018-01-05 10:03 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jan 05, 2018 at 09:44:39AM +1100, Jonathan Liu wrote: > Hi Maxime, > > On 5 January 2018 at 06:56, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote: > >> We should check if the best match has been set before comparing it. > >> > >> Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") > >> Signed-off-by: Jonathan Liu <net147@gmail.com> > >> --- > >> drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c > >> index dc332ea56f6c..4d235e5ea31c 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c > >> @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, > >> goto out; > >> } > >> > >> - if (abs(rate - rounded / i) < > >> + if (!best_parent || abs(rate - rounded / i) < > > > > Why is that causing any issue? > > > > If best_parent is set to 0... > > > >> abs(rate - best_parent / best_div)) { > > > > ... the value returned here is going to be rate, which is going to be > > higher than the first part of the comparison meaning ... > > > >> best_parent = rounded; > > > > ... that best_parent is going to be set there. > > Consider the following: > rate = 83500000 > rounded = ideal * 2 > > It is possible that if "rounded = clk_hw_round_rate(parent, ideal)" > gives high enough values that the condition "abs(rate - rounded / i) < > abs(rate - best_parent / best_div)" is never met. > > Then you can end up with: > req->rate = 0 > req->best_parent_rate = 0 > req->best_parent_hw = ... > > Also, the sun4i_tmds_calc_divider function has a similar check. Ok. That explanation must be part of your commit log, or at least which problem you're trying to address and in which situation it will trigger. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180105/04291b5f/attachment.sig> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate 2018-01-05 10:03 ` Maxime Ripard @ 2018-01-05 11:08 ` Jonathan Liu 0 siblings, 0 replies; 8+ messages in thread From: Jonathan Liu @ 2018-01-05 11:08 UTC (permalink / raw) To: linux-arm-kernel Hi Maxime, On 5 January 2018 at 21:03, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Fri, Jan 05, 2018 at 09:44:39AM +1100, Jonathan Liu wrote: >> On 5 January 2018 at 06:56, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >> > On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote: >> >> We should check if the best match has been set before comparing it. >> >> >> >> Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") >> >> Signed-off-by: Jonathan Liu <net147@gmail.com> >> >> --- >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> >> index dc332ea56f6c..4d235e5ea31c 100644 >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> >> @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, >> >> goto out; >> >> } >> >> >> >> - if (abs(rate - rounded / i) < >> >> + if (!best_parent || abs(rate - rounded / i) < >> > >> > Why is that causing any issue? >> > >> > If best_parent is set to 0... >> > >> >> abs(rate - best_parent / best_div)) { >> > >> > ... the value returned here is going to be rate, which is going to be >> > higher than the first part of the comparison meaning ... >> > >> >> best_parent = rounded; >> > >> > ... that best_parent is going to be set there. >> >> Consider the following: >> rate = 83500000 >> rounded = ideal * 2 >> >> It is possible that if "rounded = clk_hw_round_rate(parent, ideal)" >> gives high enough values that the condition "abs(rate - rounded / i) < >> abs(rate - best_parent / best_div)" is never met. >> >> Then you can end up with: >> req->rate = 0 >> req->best_parent_rate = 0 >> req->best_parent_hw = ... >> >> Also, the sun4i_tmds_calc_divider function has a similar check. > > Ok. That explanation must be part of your commit log, or at least > which problem you're trying to address and in which situation it will > trigger. Ok. I have added amended the commit message with explanation for v3. Regards, Jonathan ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate 2017-12-26 11:12 [PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate Jonathan Liu 2017-12-26 11:12 ` [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate Jonathan Liu @ 2017-12-26 11:12 ` Jonathan Liu 2017-12-26 11:12 ` [PATCH v2 3/3] drm/sun4i: hdmi: Add missing rate halving check " Jonathan Liu 2 siblings, 0 replies; 8+ messages in thread From: Jonathan Liu @ 2017-12-26 11:12 UTC (permalink / raw) To: linux-arm-kernel best_div is set to i which corresponds to rate halving when it should be set to j which corresponds to the divider. Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu <net147@gmail.com> --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index 4d235e5ea31c..88eeeaf34638 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -105,7 +105,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, if (!best_parent || abs(rate - rounded / i) < abs(rate - best_parent / best_div)) { best_parent = rounded; - best_div = i; + best_div = j; } } } -- 2.15.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] drm/sun4i: hdmi: Add missing rate halving check in sun4i_tmds_determine_rate 2017-12-26 11:12 [PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate Jonathan Liu 2017-12-26 11:12 ` [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate Jonathan Liu 2017-12-26 11:12 ` [PATCH v2 2/3] drm/sun4i: hdmi: Fix incorrect assignment " Jonathan Liu @ 2017-12-26 11:12 ` Jonathan Liu 2 siblings, 0 replies; 8+ messages in thread From: Jonathan Liu @ 2017-12-26 11:12 UTC (permalink / raw) To: linux-arm-kernel It was only checking the divider when determing the closest match if it could not match the requested rate exactly. For a projector connected to an Olimex A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and pixel clock of 83.5 MHz, this resulted in 1280x800 mode not being available and the following in dmesg when the kernel is booted with drm.debug=0x3e: [drm:drm_mode_debug_printmodeline] Modeline 37:"1280x800" 60 83500 1280 1352 1480 1680 800 810 816 831 0x48 0x5 [drm:drm_mode_prune_invalid] Not using 1280x800 mode: NOCLOCK Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu <net147@gmail.com> --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index 88eeeaf34638..3ecffa52c814 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -102,9 +102,12 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, goto out; } - if (!best_parent || abs(rate - rounded / i) < - abs(rate - best_parent / best_div)) { + if (!best_parent || + abs(rate - rounded / i / j) < + abs(rate - best_parent / best_half / + best_div)) { best_parent = rounded; + best_half = i; best_div = j; } } -- 2.15.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-01-05 11:08 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-26 11:12 [PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate Jonathan Liu 2017-12-26 11:12 ` [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate Jonathan Liu 2018-01-04 19:56 ` Maxime Ripard 2018-01-04 22:44 ` Jonathan Liu 2018-01-05 10:03 ` Maxime Ripard 2018-01-05 11:08 ` Jonathan Liu 2017-12-26 11:12 ` [PATCH v2 2/3] drm/sun4i: hdmi: Fix incorrect assignment " Jonathan Liu 2017-12-26 11:12 ` [PATCH v2 3/3] drm/sun4i: hdmi: Add missing rate halving check " Jonathan Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).