drm/exynos: fimd: vrefresh is zero

[Tobias Jakobi]

Hey Daniel,


Daniel Vetter wrote:
> On Mon, May 02, 2016 at 04:30:37PM +0200, Tobias Jakobi wrote:
>> Hello Alex,
>>
>>
>> Alex Deucher wrote:
>>> On Sat, Apr 30, 2016 at 5:37 PM, Tobias Jakobi
>>> <tjakobi at math.uni-bielefeld.de> wrote:
>>>> Hello,
>>>>
>>>> while playing around with FIMD enabled, I noticed that when first using
>>>> the device a zero division was triggered in fimd_calc_clkdiv(). I
>>>> remembered that I had a similar issue some time ago.
>>>>
>>>> I added a stub fimd_atomic_check() which shows that vrefresh is zero
>>>> when fimd_calc_clkdiv() is called.
>>>>
>>>> [  164.059361] [drm:exynos_plane_mode_set] plane : offset_x/y(0,0),
>>>> width/height(1366,768)
>>>> [  164.067175] [drm:fimd_atomic_check] xres=1366, yres=768, refresh=0,
>>>> intl=0
>>>> [  164.074198] [drm:drm_atomic_helper_check_planes] [CRTC:24:crtc-0]
>>>> atomic driver check failed
>>>>
>>>> I went back to the git log and noticed that some time ago in
>>>> 50bbfbffa5c894def440ce8157dfe53e60960d35 the fimd_mode_fixup() call was
>>>> removed.
>>>>
>>>> I'm now wondering where exactly vrefresh is set to a sane value. As far
>>>> as I can see of_get_videomode() is used to fetch the video mode from the
>>>> DT, and drm_display_mode_from_videomode() is used to convert it. However
>>>> vrefresh is nowhere set.
>>>>
>>>> So is something broken here, or am I missing something?
>>>>
>>>
>>> There is no guarantee that vrefresh is actually set.  I think the only
>>> way to reliably get it is to call drm_mode_vrefresh().
>> Well, my impression from studying the code currently is that vrefresh is
>> _never_ set. In particular the Exynos specific DRM code only ever reads
>> vrefresh.
>>
>> I was just wondering how this is supposed to function at all.
> 
> drm_mode_set_crtcinfo() is meant to be used to fill in all the derived
> values. We might or might not want to have a default call for that in
> atomic helpers actually (before we call down into any of the driver's
> check functions for the first time). There's a bunch of flags to control
> it, but drivers with special needs could simply call it once more and
> overwrite the computed values. No harm done.
I had a look at drm_mode_set_crtcinfo() (from 4.6-rc6) but it also
doesn't set the vrefresh field. Maybe you mean a different call?

With best wishes,
Tobias


> Still need to audit all callers to make sure we don't break a driver. But
> shouldn't be too much work.
> -Daniel
>