Message-ID: <d5b3f350765165299c60ab6abfae6ee8@manjaro.org>
Date: Tue, 09 Jul 2024 18:26:20 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Andy Yan <andyshrk@....com>
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: [PATCH] drm/rockchip: cdn-dp: Remove redundant workarounds for
 firmware loading

Hello Andy,

On 2024-07-09 12:46, Andy Yan wrote:
> At 2024-07-09 18:10:51, "Dragan Simic" <dsimic@...jaro.org> wrote:
>> On 2024-07-09 11:10, Andy Yan wrote:
>>> At 2024-07-09 16:17:06, "Dragan Simic" <dsimic@...jaro.org> wrote:
>>>> 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.
>> 
>> When Buildroot builds the drivers statically into the kernel image,
>> can it also be told to build the required firmware blobs into the
>> kernel image, for which there's already support in the kernel?
> 
> I‘m not sure about that。Firmware and linux kernel are two seperate
> project or repository。
> And i’m also not sure if that needs the support of the specific driver
> to build the firmware into the kernel？ >

Please see the link below, which I actually referred to.  That feature
should allow required firmware blobs to be built into the kernel image,
although I haven't tested it myself.

https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst

>> Of course, that would be feasible if only a small number of firmware
>> blobs would end up built into the kernel image, i.e. if the Buildroot
>> build would be tailored for a specific board.
>> 
>> Otherwise...
>> 
>>>> 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.
>> 
>> ... we'll need this as a systemic solution.
>> 
>>>>>> [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;
>>>>>>>> 
>> 
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Powered by blists - more mailing lists