Re: [PATCH 2/2] drm/exynos: WARN_ON if ideal_clk is zero

[Tobias Jakobi <tjakobi@math.uni-bielefeld.de>]

On 2015-05-20 18:14, Daniel Stone wrote:
> Hi,
> 
> On 20 May 2015 at 17:04, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> 
> wrote:
>> Hmm,
>> 
>> I wonder if that really 'fixes' anything, because now we get a WARN_ON 
>> which
>> is immediately followed by a div-by-zero. Furthermore we then still 
>> use the
>> result of that operation as input for a hw register (bad idea?)
>> 
>> I thought of something like this. Change fimd_calc_clkdiv() to return 
>> a
>> signed value (this shouldn't be a problem, since the returned number 
>> range
>> is small anywary). Let fimd_calc_clkdiv() return -1 when it encounters 
>> the
>> 'ideal_clk == 0' case. Move computation of clkdiv in fimd_commit() to 
>> the
>> top of the function (right after the {h,v}total checks). If 'clkdiv == 
>> -1'
>> case is encountered, then do WARN_ON and return immediately from
>> fimd_commit().
>> 
>> Should I prepare a patch for this, or does someone see an issue with 
>> this
>> approach?
> 
> Commit should never fail; as I said earlier, the right fix is to
> reject these modes during checking, by making sure that any mode which
> passes mode_fixup() and/or mode_valid() never trips this condition.
But then in this case it wouldn't help to call drm_mode_vrefresh() 
during fixup, since it still returns zero, so we're in the same 
situation as before.

I even wonder if fixup is doing anything at all. See my previous log in:
http://www.spinics.net/lists/linux-samsung-soc/msg44683.html

This is with the old code in fixup that should set vrefresh to 60.

And we see that it's actually called:
[  135.978878] [drm:fimd_mode_fixup] vrefresh 0

But then a small while later:
[  135.979048] [drm:fimd_calc_clkdiv] vrefresh 0

So apparantly nothing was fixed up here anyway?!

With best wishes,
Tobias




> 
> Cheers,
> Daniel