drm/exynos: fimd: vrefresh is zero

drm/exynos: fimd: vrefresh is zero

[Tobias Jakobi]

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?

With best wishes,
Tobias

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm/exynos: fimd: vrefresh is zero

[Alex Deucher]

On Sat, Apr 30, 2016 at 5:37 PM, Tobias Jakobi
<tjakobi@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().


Alex

Re: drm/exynos: fimd: vrefresh is zero

[Tobias Jakobi]

Hello Alex,


Alex Deucher wrote:
> On Sat, Apr 30, 2016 at 5:37 PM, Tobias Jakobi
> <tjakobi@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.

- Tobias


> 
> 
> Alex
> 

Re: drm/exynos: fimd: vrefresh is zero

[Daniel Vetter]

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@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.

Still need to audit all callers to make sure we don't break a driver. But
shouldn't be too much work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: 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@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
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm/exynos: fimd: vrefresh is zero

[Daniel Vetter]

On Mon, May 2, 2016 at 10:30 PM, Tobias Jakobi
<tjakobi@math.uni-bielefeld.de> wrote:
>> 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?

I was blind - it fills in everything except vrefresh. If you look at
drivers most of them use vrefresh purely for debug output or as an
informational field in general. Most hw wants the actual pixelclock
anyway, so there's not really a need for vrefresh (e.g. i915 only
computes vrefresh for the internally used panel modes). The value
itself is directly taken from userspace, so probably just a bug in
exynos userspace? Of course the driver should still keep working. From
a quick look you want to remove any use of vrefresh (except debug
output), and especially you shouldn't recompute the actual pixelclock
like decon/fimd seem to do (stored into ideal_clock). Instead use
mode->clock, that's supposed to be the real value.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: drm/exynos: fimd: vrefresh is zero

[Andrzej Hajda]

Hi Tobias, Daniel,

Little late, but maybe helpful anyway...

On 05/02/2016 11:00 PM, Daniel Vetter wrote:
> On Mon, May 2, 2016 at 10:30 PM, Tobias Jakobi
> <tjakobi@math.uni-bielefeld.de> wrote:
>>> 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?
> 
> I was blind - it fills in everything except vrefresh. If you look at
> drivers most of them use vrefresh purely for debug output or as an
> informational field in general. Most hw wants the actual pixelclock
> anyway, so there's not really a need for vrefresh (e.g. i915 only
> computes vrefresh for the internally used panel modes). The value
> itself is directly taken from userspace, so probably just a bug in
> exynos userspace? Of course the driver should still keep working. From
> a quick look you want to remove any use of vrefresh (except debug
> output), and especially you shouldn't recompute the actual pixelclock
> like decon/fimd seem to do (stored into ideal_clock). Instead use
> mode->clock, that's supposed to be the real value.
> -Daniel
> 

Regarding the initial question, mode->vrefresh is set in
drm_helper_probe_single_connector_modes. I guess division by zero could
be caused by lack of clock in timings provided by dts timing node.
As I remember in the past dts timings were passed without provided
clock, in such case driver calculated mode->clock for some default value
of vrefresh (60Hz for fimd if I remember correctly). Later validation
code was added and the clock should be always provided.
DTS files were updated after some time and the recalculation code was
removed from fimd driver. So it is possible you are a victim of this change.

Regarding usage by drivers, from a quick look it seems to be used for
something more than debugging by:
- amdgpu,
- exynos,
- adv7511,
- intel_dp(?),
- mdp5_cmd_encoder(?),
- nouveau,
- omap(?),
- tilcdc.

Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm/exynos: fimd: vrefresh is zero

[Andrzej Hajda]

On 05/05/2016 11:12 AM, Andrzej Hajda wrote:
> 
> Regarding the initial question, mode->vrefresh is set in
> drm_helper_probe_single_connector_modes. I guess division by zero could
> be caused by lack of clock in timings provided by dts timing node.
> As I remember in the past dts timings were passed without provided
> clock, in such case driver calculated mode->clock for some default value
> of vrefresh (60Hz for fimd if I remember correctly). Later validation
> code was added and the clock should be always provided.
> DTS files were updated after some time and the recalculation code was
> removed from fimd driver. So it is possible you are a victim of this change.

One more thing, I have looked at Exynos DTS files in mainline. In case
of exynos5*.dts files clock-frequency is set to 50000, it is of course
insanely low value and vrefresh calculated for such timings will be 0.
So please verify if clock-frequency is not too low in your case.

Regards
Andrzej

> 
> Regarding usage by drivers, from a quick look it seems to be used for
> something more than debugging by:
> - amdgpu,
> - exynos,
> - adv7511,
> - intel_dp(?),
> - mdp5_cmd_encoder(?),
> - nouveau,
> - omap(?),
> - tilcdc.
> 
> Regards
> Andrzej
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: drm/exynos: fimd: vrefresh is zero

[Tobias Jakobi]

Hello Andrzej,


Andrzej Hajda wrote:
> On 05/05/2016 11:12 AM, Andrzej Hajda wrote:
>>
>> Regarding the initial question, mode->vrefresh is set in
>> drm_helper_probe_single_connector_modes. I guess division by zero could
>> be caused by lack of clock in timings provided by dts timing node.
>> As I remember in the past dts timings were passed without provided
>> clock, in such case driver calculated mode->clock for some default value
>> of vrefresh (60Hz for fimd if I remember correctly). Later validation
>> code was added and the clock should be always provided.
>> DTS files were updated after some time and the recalculation code was
>> removed from fimd driver. So it is possible you are a victim of this change.
> 
> One more thing, I have looked at Exynos DTS files in mainline. In case
> of exynos5*.dts files clock-frequency is set to 50000, it is of course
> insanely low value and vrefresh calculated for such timings will be 0.
> So please verify if clock-frequency is not too low in your case.
I'm pretty sure that this is not the case for me.

This is the FIMD entry in my DTS:
&fimd {
        status = "okay";

        pinctrl-0 = <&lcd_clk &lcd_data24 &pwm1_out>;
        pinctrl-names = "default";

        display-timings {
                native-mode = <&timing0>;

                timing0: timing {
                        clock-frequency = <85860000>;
                        hactive = <1366>;
                        vactive = <768>;
                        hfront-porch = <48>;
                        hback-porch = <80>;
                        hsync-len = <32>;
                        vback-porch = <14>;
                        vfront-porch = <3>;
                        vsync-len = <5>;
                        pixelclk-active = <1>;
                        hsync-active = <1>;
                        vsync-active = <1>;
                        de-active = <0>;
                };
        };
};


The only peculiarity is that there is nothing attached to the external
connector on the board (ODROID-X2).

With regard to use of the vrefresh field by the various drivers, I was
made aware of Daniel on IRC that this is incorrect use.

He pointed me to:
http://lxr.free-electrons.com/source/include/drm/drm_modes.h#L374
(-> "Not used in a functional way.")

I'm going to send a patch shortly.

With best wishes,
Tobias

P.S.: I'm also experiencing another (more serious) issue, but more about
that in another thread.



> 
> Regards
> Andrzej
> 
>>
>> Regarding usage by drivers, from a quick look it seems to be used for
>> something more than debugging by:
>> - amdgpu,
>> - exynos,
>> - adv7511,
>> - intel_dp(?),
>> - mdp5_cmd_encoder(?),
>> - nouveau,
>> - omap(?),
>> - tilcdc.
>>
>> Regards
>> Andrzej
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel