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 >
