Re: [PATCH] drm: tilcdc: Calculate the vrefresh if it is not set by userspace

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxx> # v4.11+
>>> Signed-off-by: Kevin Hao <kexin.hao@xxxxxxxxxxxxx>
>>> ---
>>>  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;
>>>  }
>>>  
>>>
>>
>>


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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux