Re:Re: [PATCH] drm/rockchip: cdn-dp: Remove redundant workarounds for firmware loading

[Andy Yan]

Hi Dragan，
At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@xxxxxxxxxxx> wrote:
>Hello Andy,
>
>On 2024-07-04 04:10, Andy Yan wrote:
>> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@xxxxxxxxxxx> 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@xxxxxxxxxx/
[2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@xxxxxxxxxxxx-host-address-is-not-set/T/


>
>[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@xxxxxxxxxxx>
>>> ---
>>> 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;
>>>