Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751463AbeAELIg (ORCPT + 1 other); Fri, 5 Jan 2018 06:08:36 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:46511 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750992AbeAELIf (ORCPT ); Fri, 5 Jan 2018 06:08:35 -0500 X-Google-Smtp-Source: ACJfBosXNdWlryb6HNetqdYo+1yHPYLA2gIWdyXIrP+JWWPnWcemyT1Q511WRWqBfze6ScoHnQzANqxmv5uBFBN+ONQ= MIME-Version: 1.0 In-Reply-To: <20180105100314.3j5zm4ushv4tfky5@flea.lan> References: <20171226111227.4526-1-net147@gmail.com> <20171226111227.4526-2-net147@gmail.com> <20180104195650.vmbooz3pwjy77wt7@flea.lan> <20180105100314.3j5zm4ushv4tfky5@flea.lan> From: Jonathan Liu Date: Fri, 5 Jan 2018 22:08:33 +1100 Message-ID: Subject: Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate To: Maxime Ripard Cc: David Airlie , Chen-Yu Tsai , linux-kernel , dri-devel , linux-arm-kernel , linux-sunxi Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi Maxime, On 5 January 2018 at 21:03, Maxime Ripard wrote: > On Fri, Jan 05, 2018 at 09:44:39AM +1100, Jonathan Liu wrote: >> On 5 January 2018 at 06:56, Maxime Ripard >> 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 >> >> --- >> >> 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