Message-ID: <909d072.9028.19096c2429a.Coremail.andyshrk@163.com>
Date: Tue, 9 Jul 2024 17:10:31 +0800 (CST)
From: "Andy Yan" <andyshrk@....com>
To: "Dragan Simic" <dsimic@...jaro.org>
Cc: linux-rockchip@...ts.infradead.org, dri-devel@...ts.freedesktop.org, 
	heiko@...ech.de, hjc@...k-chips.com, andy.yan@...k-chips.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, 
	tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	javierm@...hat.com
Subject: Re:Re: [PATCH] drm/rockchip: cdn-dp: Remove redundant workarounds
 for firmware loading



Hi Draqan,
At 2024-07-09 16:17:06, "Dragan Simic" <dsimic@...jaro.org> wrote:
>Hello Andy,
>
>On 2024-07-08 09:46, Andy Yan wrote:
>> At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@...jaro.org> wrote:
>>> On 2024-07-04 04:10, Andy Yan wrote:
>>>> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@...jaro.org> wrote:
>>>>> After the additional firmware-related module information was
>>>>> introduced by
>>>>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add
>>>>> MODULE_FIRMWARE
>>>>> macro"), there's no longer need for the firmware-loading workarounds
>>>>> whose
>>>>> sole purpose was to prevent the missing firmware blob in an initial
>>>>> ramdisk
>>>>> from causing driver initialization to fail.  Thus, delete the
>>>>> workarounds,
>>>>> which removes a sizable chunk of redundant code.
>>>> 
>>>> What would happen if there was no ramdisk? And the firmware is in
>>>> rootfs ？
>>>> 
>>>> For example： A buildroot based tiny embedded system。
>>> 
>>> Good point, let me explain, please.
>>> 
>>> In general, if a driver is built into the kernel, there should also be
>>> an initial ramdisk that contains the related firmware blobs, because
>>> it's
>>> unknown is the root filesystem available when the driver is probed.  
>>> If
>>> a driver is built as a module and there's no initial ramdisk, having
>>> the related firmware blobs on the root filesystem should be fine,
>>> because
>>> the firmware blobs and the kernel module become available at the same
>>> time, through the root filesystem. [1]
>>> 
>>> Another option for a driver built statically into the kernel, when
>>> there's
>>> no initial ramdisk, is to build the required firmware blobs into the
>>> kernel
>>> image. [2]  Of course, that's feasible only when a kernel image is 
>>> built
>>> specificially for some device, because otherwise it would become too
>>> large
>>> because of too many drivers and their firmware blobs becoming 
>>> included,
>>> but that seems to fit the Buildroot-based example.
>>> 
>>> To sum it up, mechanisms already exist in the kernel for various
>>> scenarios
>>> when it comes to loading firmware blobs.  Even if the deleted 
>>> workaround
>>> attempts to solve some issue specific to some environment, that isn't
>>> the
>>> right place or the right way for solving any issues of that kind.
>>> 
>>> While preparing this patch, I even tried to find another kernel driver
>>> that
>>> also implements some similar workarounds for firmware loading, to
>>> justify
>>> the existence of such workarounds and to possibly move them into the
>>> kernel's
>>> firmware-loading interface.  Alas, I was unable to find such 
>>> workarounds
>>> in
>>> other drivers, which solidified my reasoning behind classifying the
>>> removed
>>> code as out-of-place and redundant.
>> 
>> For some tiny embedded system，there is no such ramdisk，for example：
>> a buildroot based rootfs，the buildroot only generate rootfs。
>> 
>> And FYI， there are mainline drivers try to fix such issue by
>> defer_probe，for example：
>> smc_abc[0]
>> There are also some other similar scenario in gpu driver{1}[2]
>> 
>> [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518
>> [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/
>> [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/
>
>Thanks for providing these examples.
>
>Before I continue thinking about the possible systemic solution,
>could you please clarify the way Buildroot builds the kernel and
>prepares the root filesystem?  I'm not familiar with Buildroot,
>but it seems to me that it builds the drivers statically into the
>produced kernel image, while it places the related firmware blobs
>into the produced root filesystem.  Am I right there?

in practice we can chose build the drivers statically into the kernel，
we can also build it as a module。
And in both case， the firmware blobs are put in rootfs。
If the drivers is built as a module， the module will also put in rootfs，
so its fine。
But if a drivers is built into the kernel ，it maybe can't access the firmware blob
before the rootfs is mounted.
So we can see some drivers try to use  DEFER_PROBE to fix this issue.


>
>As I already wrote earlier, and as the above-linked discussions
>conclude, solving these issues doesn't belong to any specific driver.
>It should be resolved within the kernel's firmware loading mechanism
>instead, and no driver should be specific in that regard.

IT would be good if it can be resolved within the kernel's  firmware loading mechanism.

>
>>> [1] 
>>> https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst
>>> [2] 
>>> https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst
>>> 
>>>>> Various utilities used by Linux distributions to generate initial
>>>>> ramdisks
>>>>> need to obey the firmware-related module information, so we can rely
>>>>> on the
>>>>> firmware blob being present in the generated initial ramdisks.
>>>>> 
>>>>> Signed-off-by: Dragan Simic <dsimic@...jaro.org>
>>>>> ---
>>>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 
>>>>> +++++---------------------
>>>>> 1 file changed, 10 insertions(+), 43 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>>> index bd7aa891b839..e1a7c6a1172b 100644
>>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>>>> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device
>>>>> *encoder_to_dp(struct drm_encoder *encoder)
>>>>> #define DPTX_HPD_DEL		(2 << 12)
>>>>> #define DPTX_HPD_SEL_MASK	(3 << 28)
>>>>> 
>>>>> -#define CDN_FW_TIMEOUT_MS	(64 * 1000)
>>>>> #define CDN_DPCD_TIMEOUT_MS	5000
>>>>> #define CDN_DP_FIRMWARE		"rockchip/dptx.bin"
>>>>> +
>>>>> MODULE_FIRMWARE(CDN_DP_FIRMWARE);
>>>>> 
>>>>> struct cdn_dp_data {
>>>>> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct
>>>>> cdn_dp_device *dp,
>>>>> 	return PTR_ERR_OR_ZERO(dp->audio_pdev);
>>>>> }
>>>>> 
>>>>> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp)
>>>>> -{
>>>>> -	int ret;
>>>>> -	unsigned long timeout = jiffies +
>>>>> msecs_to_jiffies(CDN_FW_TIMEOUT_MS);
>>>>> -	unsigned long sleep = 1000;
>>>>> -
>>>>> -	WARN_ON(!mutex_is_locked(&dp->lock));
>>>>> -
>>>>> -	if (dp->fw_loaded)
>>>>> -		return 0;
>>>>> -
>>>>> -	/* Drop the lock before getting the firmware to avoid blocking 
>>>>> boot
>>>>> */
>>>>> -	mutex_unlock(&dp->lock);
>>>>> -
>>>>> -	while (time_before(jiffies, timeout)) {
>>>>> -		ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev);
>>>>> -		if (ret == -ENOENT) {
>>>>> -			msleep(sleep);
>>>>> -			sleep *= 2;
>>>>> -			continue;
>>>>> -		} else if (ret) {
>>>>> -			DRM_DEV_ERROR(dp->dev,
>>>>> -				      "failed to request firmware: %d\n", ret);
>>>>> -			goto out;
>>>>> -		}
>>>>> -
>>>>> -		dp->fw_loaded = true;
>>>>> -		ret = 0;
>>>>> -		goto out;
>>>>> -	}
>>>>> -
>>>>> -	DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n");
>>>>> -	ret = -ETIMEDOUT;
>>>>> -out:
>>>>> -	mutex_lock(&dp->lock);
>>>>> -	return ret;
>>>>> -}
>>>>> -
>>>>> static void cdn_dp_pd_event_work(struct work_struct *work)
>>>>> {
>>>>> 	struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device,
>>>>> 						event_work);
>>>>> 	struct drm_connector *connector = &dp->connector;
>>>>> 	enum drm_connector_status old_status;
>>>>> -
>>>>> 	int ret;
>>>>> 
>>>>> 	mutex_lock(&dp->lock);
>>>>> 
>>>>> 	if (dp->suspended)
>>>>> 		goto out;
>>>>> 
>>>>> -	ret = cdn_dp_request_firmware(dp);
>>>>> -	if (ret)
>>>>> -		goto out;
>>>>> +	if (!dp->fw_loaded) {
>>>>> +		ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev);
>>>>> +		if (ret) {
>>>>> +			DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret);
>>>>> +			goto out;
>>>>> +		}
>>>>> +
>>>>> +		dp->fw_loaded = true;
>>>>> +	}
>>>>> 
>>>>> 	dp->connected = true;
>>>>> 

Powered by blists - more mailing lists