Re: [PATCH] drm/Makefile: Move tiny drivers before native drivers

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 08 Nov 2023, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi,
>
> thanks for the patch.
>
> Am 08.11.23 um 03:46 schrieb Huacai Chen:
>> After commit 60aebc9559492cea ("drivers/firmware: Move sysfb_init() from
>> device_initcall to subsys_initcall_sync") some Lenovo laptops get a blank
>> screen until the display manager starts.
>> 
>> This regression occurs with such a Kconfig combination:
>> CONFIG_SYSFB=y
>> CONFIG_SYSFB_SIMPLEFB=y
>> CONFIG_DRM_SIMPLEDRM=y
>> CONFIG_DRM_I915=y      # Or other native drivers such as radeon, amdgpu
>> 
>> If replace CONFIG_DRM_SIMPLEDRM with CONFIG_FB_SIMPLE (they use the same
>> device), there is no blank screen. The root cause is the initialization
>> order, and this order depends on the Makefile.
>> 
>> FB_SIMPLE is before native DRM drivers (e.g. i915, radeon, amdgpu, and
>> so on), but DRM_SIMPLEDRM is after them. Thus, if we use FB_SIMPLE, I915
>> will takeover FB_SIMPLE, then no problem; and if we use DRM_SIMPLEDRM,
>> DRM_SIMPLEDRM will try to takeover I915, but fails to work.
>
> But what exactly is the problem? From the lengthy discussion threat, it 
> looks like you've stumbled across a long-known problem, where the 
> firmware driver probes a device that has already been taken by a native 
> driver. But that should not be possible.
>
> As you know, there's a platform device that represents the firmware 
> framebuffer. The firmware drivers, such as simpledrm, bind to it. In 
> i915 and the other native drivers we remove that platform device, so 
> that simpledrm does not run.

The problem is still not resolved. Another bug report at [1].

The commit message here points at 60aebc955949 ("drivers/firmware: Move
sysfb_init() from device_initcall to subsys_initcall_sync") as
regressing, and Jaak also bisected it (see Closes:).

I agree the patch here is just papering over the issue, but lacking a
proper fix, for months, a revert would be in order, no?


BR,
Jani.


[1] https://gitlab.freedesktop.org/drm/intel/-/issues/10133


>
> We call the DRM aperture helpers at [1]. It's implemented at [2]. The 
> function contains a call to sysfb_disable(), [3] which should be invoked 
> for the i915 device and remove the platform device.
>
> [1] 
> https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/i915/i915_driver.c#L489
> [2] 
> https://elixir.bootlin.com/linux/v6.5/source/drivers/video/aperture.c#L347
> [3] 
> https://elixir.bootlin.com/linux/v6.5/source/drivers/firmware/sysfb.c#L63
>
> Can you investigate why this does not work? Is sysfb_disable() not being 
> called? Does it remove the platform device?
>
>> 
>> So we can move the "tiny" directory before native DRM drivers to solve
>> this problem.
>
> Relying on linking order is just as unreliable. The usual workaround is 
> to build native drivers as modules. But first, please investigate where 
> the current code fails.
>
> Best regards
> Thomas
>
>> 
>> Fixes: 60aebc9559492cea ("drivers/firmware: Move sysfb_init() from device_initcall to subsys_initcall_sync")
>> Closes: https://lore.kernel.org/dri-devel/ZUnNi3q3yB3zZfTl@P70.localdomain/T/#t
>> Reported-by: Jaak Ristioja <jaak@ristioja.ee>
>> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
>> ---
>>   drivers/gpu/drm/Makefile | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 8e1bde059170..db0f3d3aff43 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -141,6 +141,7 @@ obj-y			+= arm/
>>   obj-y			+= display/
>>   obj-$(CONFIG_DRM_TTM)	+= ttm/
>>   obj-$(CONFIG_DRM_SCHED)	+= scheduler/
>> +obj-y			+= tiny/
>>   obj-$(CONFIG_DRM_RADEON)+= radeon/
>>   obj-$(CONFIG_DRM_AMDGPU)+= amd/amdgpu/
>>   obj-$(CONFIG_DRM_AMDGPU)+= amd/amdxcp/
>> @@ -182,7 +183,6 @@ obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu/
>>   obj-$(CONFIG_DRM_ETNAVIV) += etnaviv/
>>   obj-y			+= hisilicon/
>>   obj-y			+= mxsfb/
>> -obj-y			+= tiny/
>>   obj-$(CONFIG_DRM_PL111) += pl111/
>>   obj-$(CONFIG_DRM_TVE200) += tve200/
>>   obj-$(CONFIG_DRM_XEN) += xen/

-- 
Jani Nikula, Intel