| Message ID | 20171010120909.26290-1-kexin.hao@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 10/11/17 05:57, Kevin Hao wrote: > On Tue, Oct 10, 2017 at 04:58:15PM +0300, Jyri Sarha wrote: >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >> >> On 10/10/17 15:09, Kevin Hao wrote: >>> The check for vrefresh has been removed by commit 11abbc9f39e0 >>> ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is >>> enabled"). But the userspace is really possible to request a display >>> mode with vrefresh uninitialized, then it would cause the following >>> calltrace: >>> [<c010fa20>] (unwind_backtrace) from [<c010c508>] (show_stack+0x20/0x24) >>> [<c010c508>] (show_stack) from [<c0959744>] (dump_stack+0x20/0x28) >>> [<c0959744>] (dump_stack) from [<c010c23c>] (__div0+0x20/0x28) >>> [<c010c23c>] (__div0) from [<c095805c>] (Ldiv0+0x8/0x14) >>> [<c095805c>] (Ldiv0) from [<c0615ccc>] (tilcdc_crtc_update_fb+0xa8/0x1d4) >>> [<c0615ccc>] (tilcdc_crtc_update_fb) from [<c0614b70>] (tilcdc_plane_atomic_update+0x54/0x5c) >>> [<c0614b70>] (tilcdc_plane_atomic_update) from [<c05c53a4>] (drm_atomic_helper_commit_planes+0x1b4/0x270) >>> [<c05c53a4>] (drm_atomic_helper_commit_planes) from [<c0617d48>] (tilcdc_commit+0x58/0x84) >>> [<c0617d48>] (tilcdc_commit) from [<c05e3f08>] (drm_atomic_commit+0x54/0x68) >>> [<c05e3f08>] (drm_atomic_commit) from [<c05c937c>] (drm_atomic_helper_set_config+0x68/0x9c) >>> [<c05c937c>] (drm_atomic_helper_set_config) from [<c05d843c>] (__drm_mode_set_config_internal+0x60/0xe0) >>> [<c05d843c>] (__drm_mode_set_config_internal) from [<c05d906c>] (drm_mode_setcrtc+0x3a8/0x4c0) >>> [<c05d906c>] (drm_mode_setcrtc) from [<c05d3c2c>] (drm_ioctl_kernel+0x78/0xb0) >>> [<c05d3c2c>] (drm_ioctl_kernel) from [<c05d3e60>] (drm_ioctl+0x1fc/0x328) >>> [<c05d3e60>] (drm_ioctl) from [<c0280008>] (vfs_ioctl+0x28/0x44) >>> [<c0280008>] (vfs_ioctl) from [<c0280a00>] (do_vfs_ioctl+0x890/0x8ec) >>> [<c0280a00>] (do_vfs_ioctl) from [<c0280abc>] (SyS_ioctl+0x60/0x7c) >>> [<c0280abc>] (SyS_ioctl) from [<c01078e0>] (ret_fast_syscall+0x0/0x48) >>> >>> So calculate the vrefresh in mode_fixup() to make sure there always >>> have a valid vrefresh. >>> >> >> The earlier check was there for the frame buffer updates when the >> display is not yet enabled and the hwmode's vrefresh is not valid yet. >> Now, with the lock protected enabled state variable check, this should >> never happen. >> >> Do you have a reliable way to reproduce the problem you are seeing? > > I get the calltrace every time when I reboot my BeagleBone Black board. > The following is the xorg.conf I used: > root@beaglebone:~# cat /etc/X11/xorg.conf > Section "Monitor" > Identifier "Builtin Default Monitor" > EndSection > > Section "Device" > Identifier "Builtin Default fbdev Device 0" > Driver "modesetting" > EndSection > > Section "Screen" > Identifier "Builtin Default fbdev Screen 0" > DefaultDepth 16 > Device "Builtin Default fbdev Device 0" > Monitor "Builtin Default Monitor" > EndSection > > Section "ServerLayout" > Identifier "Builtin Default Layout" > Screen "Builtin Default fbdev Screen 0" > EndSection > >> Could try adding a test for the vrefresh validity in the >> tilcdc_crtc_mode_valid() to see if that helps? > > No, this will break the modeset driver. I have added the following code: > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index d2589f310437..4dbce75272a3 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -845,6 +845,11 @@ int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode) > return MODE_BAD; > } > > + if (!mode->vrefresh) { > + DBG("Pruning mode: invalid vrefresh"); > + return MODE_BAD; > + } > + > return MODE_OK; > } > > And got the following error for Xorg: > [ 17.563] (II) modeset(0): Damage tracking initialized > [ 17.563] (II) modeset(0): Setting screen physical size to 169 x 127 > [ 18.388] (EE) modeset(0): failed to set mode: Invalid argument > [ 21.907] (II) modeset(0): Disabling kernel dirty updates, not required. > > > It seems that function drmmode_set_mode_major() in hw/xfree86/drivers/modesetting/drmmode_display.c > always invoke the drmModeSetCrtc() with a zero vrefresh. > I first thought this is a bug either in X-server or drmlib. But then it says "Not used in a functional way." about the vrefresh in the struct drm_display_mode definition. Now I wonder if we should use the value in the driver at all. I think we should recalculate it unconditionally. Also the place where the value is calculated is currently wrong. The mode fixup is only executed if the encoder needs VESA-compliant sync. I'll make a new patch with a comment of what is going on. Best regards, Jyri > Thanks, > Kevin > >> >> Adding the extra test in the mode validity check probably produces a >> failed mode set attempt, which I guess is Ok if the mode has zero >> vertical refresh time. But if this does not help, then the issue you are >> seeing may be an indication of a synchronization problem somewhere. >> >> Best regards, >> Jyri >> >>> Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled") >>> Cc: <stable@vger.kernel.org> # v4.11+ >>> Signed-off-by: Kevin Hao <kexin.hao@windriver.com> >>> --- >>> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >>> index d2589f310437..16b020a9044c 100644 >>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >>> @@ -690,6 +690,9 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc, >>> adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC; >>> } >>> >>> + if (!adjusted_mode->vrefresh) >>> + adjusted_mode->vrefresh = drm_mode_vrefresh(adjusted_mode); >>> + >>> return true; >>> } >>> >>> >> >>
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index d2589f310437..16b020a9044c 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -690,6 +690,9 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc, adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC; } + if (!adjusted_mode->vrefresh) + adjusted_mode->vrefresh = drm_mode_vrefresh(adjusted_mode); + return true; }
The check for vrefresh has been removed by commit 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled"). But the userspace is really possible to request a display mode with vrefresh uninitialized, then it would cause the following calltrace: [<c010fa20>] (unwind_backtrace) from [<c010c508>] (show_stack+0x20/0x24) [<c010c508>] (show_stack) from [<c0959744>] (dump_stack+0x20/0x28) [<c0959744>] (dump_stack) from [<c010c23c>] (__div0+0x20/0x28) [<c010c23c>] (__div0) from [<c095805c>] (Ldiv0+0x8/0x14) [<c095805c>] (Ldiv0) from [<c0615ccc>] (tilcdc_crtc_update_fb+0xa8/0x1d4) [<c0615ccc>] (tilcdc_crtc_update_fb) from [<c0614b70>] (tilcdc_plane_atomic_update+0x54/0x5c) [<c0614b70>] (tilcdc_plane_atomic_update) from [<c05c53a4>] (drm_atomic_helper_commit_planes+0x1b4/0x270) [<c05c53a4>] (drm_atomic_helper_commit_planes) from [<c0617d48>] (tilcdc_commit+0x58/0x84) [<c0617d48>] (tilcdc_commit) from [<c05e3f08>] (drm_atomic_commit+0x54/0x68) [<c05e3f08>] (drm_atomic_commit) from [<c05c937c>] (drm_atomic_helper_set_config+0x68/0x9c) [<c05c937c>] (drm_atomic_helper_set_config) from [<c05d843c>] (__drm_mode_set_config_internal+0x60/0xe0) [<c05d843c>] (__drm_mode_set_config_internal) from [<c05d906c>] (drm_mode_setcrtc+0x3a8/0x4c0) [<c05d906c>] (drm_mode_setcrtc) from [<c05d3c2c>] (drm_ioctl_kernel+0x78/0xb0) [<c05d3c2c>] (drm_ioctl_kernel) from [<c05d3e60>] (drm_ioctl+0x1fc/0x328) [<c05d3e60>] (drm_ioctl) from [<c0280008>] (vfs_ioctl+0x28/0x44) [<c0280008>] (vfs_ioctl) from [<c0280a00>] (do_vfs_ioctl+0x890/0x8ec) [<c0280a00>] (do_vfs_ioctl) from [<c0280abc>] (SyS_ioctl+0x60/0x7c) [<c0280abc>] (SyS_ioctl) from [<c01078e0>] (ret_fast_syscall+0x0/0x48) So calculate the vrefresh in mode_fixup() to make sure there always have a valid vrefresh. Fixes: 11abbc9f39e0 ("drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled") Cc: <stable@vger.kernel.org> # v4.11+ Signed-off-by: Kevin Hao <kexin.hao@windriver.com> --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 3 +++ 1 file changed, 3 insertions(+)