diff mbox series

[v2,3/8] firmware/sysfb: Set firmware-framebuffer parent device

Message ID 20240202120140.3517-4-tzimmermann@suse.de (mailing list archive)
State Superseded
Headers show
Series firmware/sysfb: Track parent device for screen_info | expand

Commit Message

Thomas Zimmermann Feb. 2, 2024, 11:58 a.m. UTC 
Set the firmware framebuffer's parent device, which usually is the
graphics hardware's physical device. Integrates the framebuffer in
the Linux device hierarchy and lets Linux handle dependencies among
devices. For example, the graphics hardware won't be suspended while
the firmware device is still active.

v2:
	* detect parent device in sysfb_parent_dev()

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb.c          | 19 ++++++++++++++++++-
 drivers/firmware/sysfb_simplefb.c |  5 ++++-
 include/linux/sysfb.h             |  3 ++-
 3 files changed, 24 insertions(+), 3 deletions(-)

Comments

Sui Jingfeng Feb. 2, 2024, 2:40 p.m. UTC | #1 
Hi, Thomas:

I love your patch, yet it can cause the linux kernel fail to compile
if the CONFIG_SYSFB_SIMPLEFB option is not selected. I paste the log
at below for easier to read and debug. Please fix this. :-)

drivers/firmware/sysfb.c: In function ‘sysfb_init’:
drivers/firmware/sysfb.c:134:22: error: too many arguments to function 
‘sysfb_create_simplefb’
   134 |                 pd = sysfb_create_simplefb(si, &mode, parent);
       |                      ^~~~~~~~~~~~~~~~~~~~~
In file included from drivers/firmware/sysfb.c:36:
./include/linux/sysfb.h:105:39: note: declared here
   105 | static inline struct platform_device 
*sysfb_create_simplefb(const struct screen_info *si,
       | ^~~~~~~~~~~~~~~~~~~~~
make[4]: *** [scripts/Makefile.build:243: drivers/firmware/sysfb.o] Error 1
make[3]: *** [scripts/Makefile.build:481: drivers/firmware] Error 2
make[3]: *** Waiting for unfinished jobs....

This is because you forget to modify prototype of sysfb_create_simplefb() function,
Please see it in include/linux/sysfb.h, at the 106 line of the file.


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> Set the firmware framebuffer's parent device, which usually is the
> graphics hardware's physical device. Integrates the framebuffer in
> the Linux device hierarchy and lets Linux handle dependencies among
> devices. For example, the graphics hardware won't be suspended while
> the firmware device is still active.
>
> v2:
> 	* detect parent device in sysfb_parent_dev()
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/firmware/sysfb.c          | 19 ++++++++++++++++++-
>   drivers/firmware/sysfb_simplefb.c |  5 ++++-
>   include/linux/sysfb.h             |  3 ++-
>   3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> index 3c197db42c9d9..d02945b0d8ea1 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -29,6 +29,7 @@
>   #include <linux/init.h>
>   #include <linux/kernel.h>
>   #include <linux/mm.h>
> +#include <linux/pci.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
>   #include <linux/screen_info.h>
> @@ -69,9 +70,21 @@ void sysfb_disable(void)
>   }
>   EXPORT_SYMBOL_GPL(sysfb_disable);
>   
> +static __init struct device *sysfb_parent_dev(const struct screen_info *si)
> +{
> +	struct pci_dev *pdev;
> +
> +	pdev = screen_info_pci_dev(si);
> +	if (pdev)
> +		return &pdev->dev;
> +
> +	return NULL;
> +}
> +
>   static __init int sysfb_init(void)
>   {
>   	struct screen_info *si = &screen_info;
> +	struct device *parent;
>   	struct simplefb_platform_data mode;
>   	const char *name;
>   	bool compatible;
> @@ -83,10 +96,12 @@ static __init int sysfb_init(void)
>   
>   	sysfb_apply_efi_quirks();
>   
> +	parent = sysfb_parent_dev(si);
> +
>   	/* try to create a simple-framebuffer device */
>   	compatible = sysfb_parse_mode(si, &mode);
>   	if (compatible) {
> -		pd = sysfb_create_simplefb(si, &mode);
> +		pd = sysfb_create_simplefb(si, &mode, parent);
>   		if (!IS_ERR(pd))
>   			goto unlock_mutex;
>   	}
> @@ -109,6 +124,8 @@ static __init int sysfb_init(void)
>   		goto unlock_mutex;
>   	}
>   
> +	pd->dev.parent = parent;
> +
>   	sysfb_set_efifb_fwnode(pd);
>   
>   	ret = platform_device_add_data(pd, si, sizeof(*si));
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index 74363ed7501f6..75a186bf8f8ec 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -91,7 +91,8 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   }
>   
>   __init struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
> -						     const struct simplefb_platform_data *mode)
> +						     const struct simplefb_platform_data *mode,
> +						     struct device *parent)
>   {
>   	struct platform_device *pd;
>   	struct resource res;
> @@ -143,6 +144,8 @@ __init struct platform_device *sysfb_create_simplefb(const struct screen_info *s
>   	if (!pd)
>   		return ERR_PTR(-ENOMEM);
>   
> +	pd->dev.parent = parent;
> +
>   	sysfb_set_efifb_fwnode(pd);
>   
>   	ret = platform_device_add_resources(pd, &res, 1);
> diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
> index 19cb803dd5ecd..6ee3ade3f8b06 100644
> --- a/include/linux/sysfb.h
> +++ b/include/linux/sysfb.h
> @@ -91,7 +91,8 @@ static inline void sysfb_set_efifb_fwnode(struct platform_device *pd)
>   bool sysfb_parse_mode(const struct screen_info *si,
>   		      struct simplefb_platform_data *mode);
>   struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
> -					      const struct simplefb_platform_data *mode);
> +					      const struct simplefb_platform_data *mode,
> +					      struct device *parent);
>   
>   #else /* CONFIG_SYSFB_SIMPLE */
>
Sui Jingfeng Feb. 2, 2024, 3:23 p.m. UTC | #2 
Hi,


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> Set the firmware framebuffer's parent device, which usually is the
> graphics hardware's physical device. Integrates the framebuffer in
> the Linux device hierarchy and lets Linux handle dependencies among
> devices. For example, the graphics hardware won't be suspended while
> the firmware device is still active.

This is a very nice benefit, I can't agree more!

Because the backing memory of the firmware framebuffer occupied
belongs to the graphics hardware itself. For PCIe device, the
backing memory is typically the dedicated VRAM of the PCIe GPU.
But there are some exceptions, for example, the gma500. But I
think this can be fixed in the future, as majority(>99.9%) PCIe
GPU has the a dedicated VRAM.


For ARM and ARM64 platform device, the backing memory of the
firmware framebuffer may located at the system RAM. It's common
that the display controller is a platform device in the embedded
world. So I think the sysfb_parent_dev() function can be extended
to be able to works for platform device in the future.
kernel test robot Feb. 3, 2024, 7:12 p.m. UTC | #3 
Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip linus/master v6.8-rc2 next-20240202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/video-Add-helpers-for-decoding-screen_info/20240202-200314
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240202120140.3517-4-tzimmermann%40suse.de
patch subject: [PATCH v2 3/8] firmware/sysfb: Set firmware-framebuffer parent device
config: i386-buildonly-randconfig-002-20240203 (https://download.01.org/0day-ci/archive/20240204/202402040214.GFutmkRC-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240204/202402040214.GFutmkRC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402040214.GFutmkRC-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/sysfb.c: In function 'sysfb_init':
>> drivers/firmware/sysfb.c:104:8: error: too many arguments to function 'sysfb_create_simplefb'
      pd = sysfb_create_simplefb(si, &mode, parent);
           ^~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/firmware/sysfb.c:36:0:
   include/linux/sysfb.h:105:39: note: declared here
    static inline struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
                                          ^~~~~~~~~~~~~~~~~~~~~


vim +/sysfb_create_simplefb +104 drivers/firmware/sysfb.c

    83	
    84	static __init int sysfb_init(void)
    85	{
    86		struct screen_info *si = &screen_info;
    87		struct device *parent;
    88		struct simplefb_platform_data mode;
    89		const char *name;
    90		bool compatible;
    91		int ret = 0;
    92	
    93		mutex_lock(&disable_lock);
    94		if (disabled)
    95			goto unlock_mutex;
    96	
    97		sysfb_apply_efi_quirks();
    98	
    99		parent = sysfb_parent_dev(si);
   100	
   101		/* try to create a simple-framebuffer device */
   102		compatible = sysfb_parse_mode(si, &mode);
   103		if (compatible) {
 > 104			pd = sysfb_create_simplefb(si, &mode, parent);
   105			if (!IS_ERR(pd))
   106				goto unlock_mutex;
   107		}
   108	
   109		/* if the FB is incompatible, create a legacy framebuffer device */
   110		if (si->orig_video_isVGA == VIDEO_TYPE_EFI)
   111			name = "efi-framebuffer";
   112		else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
   113			name = "vesa-framebuffer";
   114		else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
   115			name = "vga-framebuffer";
   116		else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
   117			name = "ega-framebuffer";
   118		else
   119			name = "platform-framebuffer";
   120	
   121		pd = platform_device_alloc(name, 0);
   122		if (!pd) {
   123			ret = -ENOMEM;
   124			goto unlock_mutex;
   125		}
   126	
   127		pd->dev.parent = parent;
   128	
   129		sysfb_set_efifb_fwnode(pd);
   130	
   131		ret = platform_device_add_data(pd, si, sizeof(*si));
   132		if (ret)
   133			goto err;
   134	
   135		ret = platform_device_add(pd);
   136		if (ret)
   137			goto err;
   138	
   139		goto unlock_mutex;
   140	err:
   141		platform_device_put(pd);
   142	unlock_mutex:
   143		mutex_unlock(&disable_lock);
   144		return ret;
   145	}
   146
kernel test robot Feb. 4, 2024, 2:13 a.m. UTC | #4 
Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip linus/master v6.8-rc2 next-20240202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/video-Add-helpers-for-decoding-screen_info/20240202-200314
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240202120140.3517-4-tzimmermann%40suse.de
patch subject: [PATCH v2 3/8] firmware/sysfb: Set firmware-framebuffer parent device
config: i386-buildonly-randconfig-003-20240203 (https://download.01.org/0day-ci/archive/20240204/202402041001.rJrT47HE-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240204/202402041001.rJrT47HE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402041001.rJrT47HE-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/firmware/sysfb.c:104:41: error: too many arguments to function call, expected 2, have 3
     104 |                 pd = sysfb_create_simplefb(si, &mode, parent);
         |                      ~~~~~~~~~~~~~~~~~~~~~            ^~~~~~
   include/linux/sysfb.h:105:39: note: 'sysfb_create_simplefb' declared here
     105 | static inline struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
         |                                       ^                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     106 |                                                             const struct simplefb_platform_data *mode)
         |                                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +104 drivers/firmware/sysfb.c

    83	
    84	static __init int sysfb_init(void)
    85	{
    86		struct screen_info *si = &screen_info;
    87		struct device *parent;
    88		struct simplefb_platform_data mode;
    89		const char *name;
    90		bool compatible;
    91		int ret = 0;
    92	
    93		mutex_lock(&disable_lock);
    94		if (disabled)
    95			goto unlock_mutex;
    96	
    97		sysfb_apply_efi_quirks();
    98	
    99		parent = sysfb_parent_dev(si);
   100	
   101		/* try to create a simple-framebuffer device */
   102		compatible = sysfb_parse_mode(si, &mode);
   103		if (compatible) {
 > 104			pd = sysfb_create_simplefb(si, &mode, parent);
   105			if (!IS_ERR(pd))
   106				goto unlock_mutex;
   107		}
   108	
   109		/* if the FB is incompatible, create a legacy framebuffer device */
   110		if (si->orig_video_isVGA == VIDEO_TYPE_EFI)
   111			name = "efi-framebuffer";
   112		else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
   113			name = "vesa-framebuffer";
   114		else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
   115			name = "vga-framebuffer";
   116		else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
   117			name = "ega-framebuffer";
   118		else
   119			name = "platform-framebuffer";
   120	
   121		pd = platform_device_alloc(name, 0);
   122		if (!pd) {
   123			ret = -ENOMEM;
   124			goto unlock_mutex;
   125		}
   126	
   127		pd->dev.parent = parent;
   128	
   129		sysfb_set_efifb_fwnode(pd);
   130	
   131		ret = platform_device_add_data(pd, si, sizeof(*si));
   132		if (ret)
   133			goto err;
   134	
   135		ret = platform_device_add(pd);
   136		if (ret)
   137			goto err;
   138	
   139		goto unlock_mutex;
   140	err:
   141		platform_device_put(pd);
   142	unlock_mutex:
   143		mutex_unlock(&disable_lock);
   144		return ret;
   145	}
   146
Thomas Zimmermann Feb. 5, 2024, 8:24 a.m. UTC | #5 
Hi

Am 02.02.24 um 16:23 schrieb Sui Jingfeng:
> Hi,
> 
> 
> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>> Set the firmware framebuffer's parent device, which usually is the
>> graphics hardware's physical device. Integrates the framebuffer in
>> the Linux device hierarchy and lets Linux handle dependencies among
>> devices. For example, the graphics hardware won't be suspended while
>> the firmware device is still active.
> 
> This is a very nice benefit, I can't agree more!
> 
> Because the backing memory of the firmware framebuffer occupied
> belongs to the graphics hardware itself. For PCIe device, the
> backing memory is typically the dedicated VRAM of the PCIe GPU.
> But there are some exceptions, for example, the gma500. But I
> think this can be fixed in the future, as majority(>99.9%) PCIe
> GPU has the a dedicated VRAM.
> 
> 
> For ARM and ARM64 platform device, the backing memory of the
> firmware framebuffer may located at the system RAM. It's common
> that the display controller is a platform device in the embedded
> world. So I think the sysfb_parent_dev() function can be extended
> to be able to works for platform device in the future.

The current approach has been taken from efifb. It would already not 
work reliably with gma500 or ARM SoCs. So there's no immediate loss of 
functionality AFAICT. But with the patchset now have a correct device 
hierarchy and PM for simpledrm, vesafb et al.

In the long term, I want to employ some of the logic in vgaarb that 
detects the firmware default device. That needs additional work, though.

Best regards
Thomas

>
Sui Jingfeng Feb. 7, 2024, 3:34 p.m. UTC | #6 
Hi,


On 2024/2/5 16:24, Thomas Zimmermann wrote:
> Hi
>
> Am 02.02.24 um 16:23 schrieb Sui Jingfeng:
>> Hi,
>>
>>
>> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>>> Set the firmware framebuffer's parent device, which usually is the
>>> graphics hardware's physical device. Integrates the framebuffer in
>>> the Linux device hierarchy and lets Linux handle dependencies among
>>> devices. For example, the graphics hardware won't be suspended while
>>> the firmware device is still active.
>>
>> This is a very nice benefit, I can't agree more!
>>
>> Because the backing memory of the firmware framebuffer occupied
>> belongs to the graphics hardware itself. For PCIe device, the
>> backing memory is typically the dedicated VRAM of the PCIe GPU.
>> But there are some exceptions, for example, the gma500. But I
>> think this can be fixed in the future, as majority(>99.9%) PCIe
>> GPU has the a dedicated VRAM.
>>
>>
>> For ARM and ARM64 platform device, the backing memory of the
>> firmware framebuffer may located at the system RAM. It's common
>> that the display controller is a platform device in the embedded
>> world. So I think the sysfb_parent_dev() function can be extended
>> to be able to works for platform device in the future.
>
> The current approach has been taken from efifb. It would already not 
> work reliably with gma500 or ARM SoCs. So there's no immediate loss of 
> functionality AFAICT. But with the patchset now have a correct device 
> hierarchy and PM for simpledrm, vesafb et al.
>
> In the long term, I want to employ some of the logic in vgaarb that 
> detects the firmware default device. That needs additional work, though.
>

Good ideas, try to be impressive.
I probably could help to test if I'm online.


> Best regards
> Thomas
>
>>
>
diff mbox series

Patch

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 3c197db42c9d9..d02945b0d8ea1 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -29,6 +29,7 @@ 
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/pci.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
@@ -69,9 +70,21 @@  void sysfb_disable(void)
 }
 EXPORT_SYMBOL_GPL(sysfb_disable);
 
+static __init struct device *sysfb_parent_dev(const struct screen_info *si)
+{
+	struct pci_dev *pdev;
+
+	pdev = screen_info_pci_dev(si);
+	if (pdev)
+		return &pdev->dev;
+
+	return NULL;
+}
+
 static __init int sysfb_init(void)
 {
 	struct screen_info *si = &screen_info;
+	struct device *parent;
 	struct simplefb_platform_data mode;
 	const char *name;
 	bool compatible;
@@ -83,10 +96,12 @@  static __init int sysfb_init(void)
 
 	sysfb_apply_efi_quirks();
 
+	parent = sysfb_parent_dev(si);
+
 	/* try to create a simple-framebuffer device */
 	compatible = sysfb_parse_mode(si, &mode);
 	if (compatible) {
-		pd = sysfb_create_simplefb(si, &mode);
+		pd = sysfb_create_simplefb(si, &mode, parent);
 		if (!IS_ERR(pd))
 			goto unlock_mutex;
 	}
@@ -109,6 +124,8 @@  static __init int sysfb_init(void)
 		goto unlock_mutex;
 	}
 
+	pd->dev.parent = parent;
+
 	sysfb_set_efifb_fwnode(pd);
 
 	ret = platform_device_add_data(pd, si, sizeof(*si));
diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 74363ed7501f6..75a186bf8f8ec 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -91,7 +91,8 @@  __init bool sysfb_parse_mode(const struct screen_info *si,
 }
 
 __init struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
-						     const struct simplefb_platform_data *mode)
+						     const struct simplefb_platform_data *mode,
+						     struct device *parent)
 {
 	struct platform_device *pd;
 	struct resource res;
@@ -143,6 +144,8 @@  __init struct platform_device *sysfb_create_simplefb(const struct screen_info *s
 	if (!pd)
 		return ERR_PTR(-ENOMEM);
 
+	pd->dev.parent = parent;
+
 	sysfb_set_efifb_fwnode(pd);
 
 	ret = platform_device_add_resources(pd, &res, 1);
diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
index 19cb803dd5ecd..6ee3ade3f8b06 100644
--- a/include/linux/sysfb.h
+++ b/include/linux/sysfb.h
@@ -91,7 +91,8 @@  static inline void sysfb_set_efifb_fwnode(struct platform_device *pd)
 bool sysfb_parse_mode(const struct screen_info *si,
 		      struct simplefb_platform_data *mode);
 struct platform_device *sysfb_create_simplefb(const struct screen_info *si,
-					      const struct simplefb_platform_data *mode);
+					      const struct simplefb_platform_data *mode,
+					      struct device *parent);
 
 #else /* CONFIG_SYSFB_SIMPLE */