[PATCH] video/aperture: fix typos

[PATCH] video/aperture: fix typos

[Sui Jingfeng]

 EFI FB, VESA FB or VGA FB etc are belong to firmware based framebuffer
 driver.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/video/aperture.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 41e77de1ea82..b009468ffdff 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -20,7 +20,7 @@
  * driver can be active at any given time. Many systems load a generic
  * graphics drivers, such as EFI-GOP or VESA, early during the boot process.
  * During later boot stages, they replace the generic driver with a dedicated,
- * hardware-specific driver. To take over the device the dedicated driver
+ * hardware-specific driver. To take over the device, the dedicated driver
  * first has to remove the generic driver. Aperture functions manage
  * ownership of framebuffer memory and hand-over between drivers.
  *
@@ -76,7 +76,7 @@
  * generic EFI or VESA drivers, have to register themselves as owners of their
  * framebuffer apertures. Ownership of the framebuffer memory is achieved
  * by calling devm_aperture_acquire_for_platform_device(). If successful, the
- * driveris the owner of the framebuffer range. The function fails if the
+ * driver is the owner of the framebuffer range. The function fails if the
  * framebuffer is already owned by another driver. See below for an example.
  *
  * .. code-block:: c
@@ -126,7 +126,7 @@
  * et al for the registered framebuffer range, the aperture helpers call
  * platform_device_unregister() and the generic driver unloads itself. The
  * generic driver also has to provide a remove function to make this work.
- * Once hot unplugged fro mhardware, it may not access the device's
+ * Once hot unplugged from hardware, it may not access the device's
  * registers, framebuffer memory, ROM, etc afterwards.
  */
 
@@ -203,7 +203,7 @@ static void aperture_detach_platform_device(struct device *dev)
 
 	/*
 	 * Remove the device from the device hierarchy. This is the right thing
-	 * to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
+	 * to do for firmware-based fb drivers, such as EFI, VESA or VGA. After
 	 * the new driver takes over the hardware, the firmware device's state
 	 * will be lost.
 	 *
-- 
2.25.1

Re: [PATCH] video/aperture: fix typos

[Javier Martinez Canillas]

Sui Jingfeng <suijingfeng@loongson.cn> writes:

>  EFI FB, VESA FB or VGA FB etc are belong to firmware based framebuffer
>  driver.
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] video/aperture: fix typos

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Sui Jingfeng <suijingfeng@loongson.cn> writes:
>
>>  EFI FB, VESA FB or VGA FB etc are belong to firmware based framebuffer
>>  driver.
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

Pushed to drm-misc (drm-misc-next). Thanks!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] video/aperture: fix typos

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2917 bytes --]

Hi

Am 04.04.23 um 06:01 schrieb Sui Jingfeng:
>   EFI FB, VESA FB or VGA FB etc are belong to firmware based framebuffer
>   driver.

No whitespaces at the beginning of the lines.

> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>   drivers/video/aperture.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index 41e77de1ea82..b009468ffdff 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -20,7 +20,7 @@
>    * driver can be active at any given time. Many systems load a generic
>    * graphics drivers, such as EFI-GOP or VESA, early during the boot process.
>    * During later boot stages, they replace the generic driver with a dedicated,
> - * hardware-specific driver. To take over the device the dedicated driver
> + * hardware-specific driver. To take over the device, the dedicated driver
>    * first has to remove the generic driver. Aperture functions manage
>    * ownership of framebuffer memory and hand-over between drivers.
>    *
> @@ -76,7 +76,7 @@
>    * generic EFI or VESA drivers, have to register themselves as owners of their
>    * framebuffer apertures. Ownership of the framebuffer memory is achieved
>    * by calling devm_aperture_acquire_for_platform_device(). If successful, the
> - * driveris the owner of the framebuffer range. The function fails if the
> + * driver is the owner of the framebuffer range. The function fails if the
>    * framebuffer is already owned by another driver. See below for an example.
>    *
>    * .. code-block:: c
> @@ -126,7 +126,7 @@
>    * et al for the registered framebuffer range, the aperture helpers call
>    * platform_device_unregister() and the generic driver unloads itself. The
>    * generic driver also has to provide a remove function to make this work.
> - * Once hot unplugged fro mhardware, it may not access the device's
> + * Once hot unplugged from hardware, it may not access the device's
>    * registers, framebuffer memory, ROM, etc afterwards.
>    */
>   
> @@ -203,7 +203,7 @@ static void aperture_detach_platform_device(struct device *dev)
>   
>   	/*
>   	 * Remove the device from the device hierarchy. This is the right thing
> -	 * to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
> +	 * to do for firmware-based fb drivers, such as EFI, VESA or VGA. After

That sentences is not well phrased. Maybe say 'This is required for 
firmware-provided graphics, such as EFI, VESA or VGA.'

Best regards
Thomas

>   	 * the new driver takes over the hardware, the firmware device's state
>   	 * will be lost.
>   	 *

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] video/aperture: fix typos

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

Sorry, I just applied this patch and didn't see your email before...

> Hi
>
> Am 04.04.23 um 06:01 schrieb Sui Jingfeng:
>>   EFI FB, VESA FB or VGA FB etc are belong to firmware based framebuffer
>>   driver.
>
> No whitespaces at the beginning of the lines.
>

I fixed that before applying, also removed the "are" in the sentence
above, since it sounded off and repharsed subject line as "Fix typos
in comments".

[...]

>>   	/*
>>   	 * Remove the device from the device hierarchy. This is the right thing
>> -	 * to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
>> +	 * to do for firmware-based fb drivers, such as EFI, VESA or VGA. After
>
> That sentences is not well phrased. Maybe say 'This is required for 
> firmware-provided graphics, such as EFI, VESA or VGA.'
>

Graphic drivers or display drivers would indeed be more accurate here. But
I think that "fb drivers" is still well pharsed since the are other places
where either fbdev or DRM drivers for firmware-provided framebuffers are
named like that.

For example, in the sysfb platform code and Kconfig symbol help text.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] video/aperture: fix typos

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

[...]

>>>   	/*
>>>   	 * Remove the device from the device hierarchy. This is the right thing
>>> -	 * to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
>>> +	 * to do for firmware-based fb drivers, such as EFI, VESA or VGA. After
>>
>> That sentences is not well phrased. Maybe say 'This is required for 
>> firmware-provided graphics, such as EFI, VESA or VGA.'
>>
>
> Graphic drivers or display drivers would indeed be more accurate here. But
> I think that "fb drivers" is still well pharsed since the are other places
> where either fbdev or DRM drivers for firmware-provided framebuffers are
> named like that.
>

Sui,

Maybe you could post a follow-up patch to improve the comment as suggested
by Thomas?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] video/aperture: fix typos

[Sui Jingfeng]

Hi,

thanks you for the time and effort  for reviewing.

On 2023/4/4 19:03, Javier Martinez Canillas wrote:
> Javier Martinez Canillas <javierm@redhat.com> writes:
>
> [...]
>
>>>>    	/*
>>>>    	 * Remove the device from the device hierarchy. This is the right thing
>>>> -	 * to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
>>>> +	 * to do for firmware-based fb drivers, such as EFI, VESA or VGA. After
>>> That sentences is not well phrased. Maybe say 'This is required for
>>> firmware-provided graphics, such as EFI, VESA or VGA.'
>>>
>> Graphic drivers or display drivers would indeed be more accurate here. But
>> I think that "fb drivers" is still well pharsed since the are other places
>> where either fbdev or DRM drivers for firmware-provided framebuffers are
>> named like that.
>>
> Sui,
>
> Maybe you could post a follow-up patch to improve the comment as suggested
> by Thomas?
>
Yes, certainly.


This is the right thing to do for conflicting drivers takes over the 
hardware resource required.


But the comments is actually nearly perfect in overall, it has some 
difficulty to improve

the perfection.  Below is my personal understanding toward the above 
sentence.


efifb and simplefb belong to the class of firmware based framebuffer driver.

They are generic and platform agnostic, yet they have to relay on the 
firmware

to passing fb format, fb size, fb base address, fb resolution and fb 
stride etc to the kernel.

Linux kernel using those information to fill the global screen_info 
structure.

sysfb_init() then using the global screen_info to  create a platform device,

the device will be claimed by efifb or simplefb driver finally. This is 
a hand over solution.

It relay on the firmware setup such a framebuffer and hand over the 
state(this is

actually a kind of modeset state) to kernel.


efifb only own the potential hardware resource for a very short time if a

conflicting drm driver probe successfully.


For the platform/graphics without  a drm driver, developers may choose to

use efifb driver as a replacement.  So, there are no conflicting happen on

such a case. The `nomodeset` kernel cmd options can also be used for

debugging and testing purpose if the more intelligent drm driver is broken

due to bugs.

Re: [PATCH] video/aperture: fix typos

[Sui Jingfeng]

On 2023/4/5 17:55, Sui Jingfeng wrote:
> Hi,
>
> thanks you for the time and effort  for reviewing.
>
> On 2023/4/4 19:03, Javier Martinez Canillas wrote:
>> Javier Martinez Canillas <javierm@redhat.com> writes:
>>
>> [...]
>>
>>>>>        /*
>>>>>         * Remove the device from the device hierarchy. This is the 
>>>>> right thing
>>>>> -     * to do for firmware-based DRM drivers, such as EFI, VESA or 
>>>>> VGA. After
>>>>> +     * to do for firmware-based fb drivers, such as EFI, VESA or 
>>>>> VGA. After
>>>> That sentences is not well phrased. Maybe say 'This is required for
>>>> firmware-provided graphics, such as EFI, VESA or VGA.'
>>>>
>>> Graphic drivers or display drivers would indeed be more accurate 
>>> here. But
>>> I think that "fb drivers" is still well pharsed since the are other 
>>> places
>>> where either fbdev or DRM drivers for firmware-provided framebuffers 
>>> are
>>> named like that.
>>>
>> Sui,
>>
>> Maybe you could post a follow-up patch to improve the comment as 
>> suggested
>> by Thomas?
>>
> Yes, certainly.
>
>
> This is the right thing to do for conflicting drivers takes over the 
> hardware resource required.
>

While the `drivers` include both drm driver and the real device 
dependent framebuffer driver,

They are typically build as kernel module as oppose to the efifb and 
simplefb which is built

into kernel kernel.  Firmware framebuffer is conflict with the drm 
driver is because the memory

region they use overlaps.  If there no overlap, then no `taken over`  
will be happen.


By the way,  I'm asked to made efifb and simplefb works on LoongArch and 
Mips platform in the past.

We are using downstream kernel(linux-4.19)  at that time. efifb is ony 
works for platform with uefi

firmware support. By ensure that framebuffer locate at the address range 
of the on-board video ram(VRAM)

and passing screen_info parameters to kernel correctly, 
drm_aperture_remove_conflicting_framebuffers

will success. This required the uefi firmware engineer understand this, 
for loongson bridge chip, this need

a small trick.  Simplefb is only tested on loongson SoC platform by 
providing fb parameters in DT which

match the PMON firmware's setting. As the framebuffer may located at 
anywhere in the physical address

space, there no aperture for SoC anymore.  Should call 
aperture_remove_conflicting_devices(0, ~0, false, "drmfb")

to remove them all.


>
> But the comments is actually nearly perfect in overall, it has some 
> difficulty to improve
>
> the perfection.  Below is my personal understanding toward the above 
> sentence.
>
>
> efifb and simplefb belong to the class of firmware based framebuffer 
> driver.
>
> They are generic and platform agnostic, yet they have to relay on the 
> firmware
>
> to passing fb format, fb size, fb base address, fb resolution and fb 
> stride etc to the kernel.
>
> Linux kernel using those information to fill the global screen_info 
> structure.
>
> sysfb_init() then using the global screen_info to  create a platform 
> device,
>
> the device will be claimed by efifb or simplefb driver finally. This 
> is a hand over solution.
>
> It relay on the firmware setup such a framebuffer and hand over the 
> state(this is
>
> actually a kind of modeset state) to kernel.
>
>
> efifb only own the potential hardware resource for a very short time if a
>
> conflicting drm driver probe successfully.
>
>
> For the platform/graphics without  a drm driver, developers may choose to
>
> use efifb driver as a replacement.  So, there are no conflicting 
> happen on
>
> such a case. The `nomodeset` kernel cmd options can also be used for
>
> debugging and testing purpose if the more intelligent drm driver is 
> broken
>
> due to bugs.
>

Re: [PATCH] video/aperture: fix typos

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1918 bytes --]

Hi

Am 04.04.23 um 12:55 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
> Sorry, I just applied this patch and didn't see your email before...
> 
>> Hi
>>
>> Am 04.04.23 um 06:01 schrieb Sui Jingfeng:
>>>    EFI FB, VESA FB or VGA FB etc are belong to firmware based framebuffer
>>>    driver.
>>
>> No whitespaces at the beginning of the lines.
>>
> 
> I fixed that before applying, also removed the "are" in the sentence
> above, since it sounded off and repharsed subject line as "Fix typos
> in comments".
> 
> [...]
> 
>>>    	/*
>>>    	 * Remove the device from the device hierarchy. This is the right thing
>>> -	 * to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
>>> +	 * to do for firmware-based fb drivers, such as EFI, VESA or VGA. After
>>
>> That sentences is not well phrased. Maybe say 'This is required for
>> firmware-provided graphics, such as EFI, VESA or VGA.'
>>
> 
> Graphic drivers or display drivers would indeed be more accurate here. But
> I think that "fb drivers" is still well pharsed since the are other places
> where either fbdev or DRM drivers for firmware-provided framebuffers are
> named like that.

I meant my original comment when I said 'not well phrased'. It's not 
Jingfeng's fault, but in my original text. Removing the device is 
required for scanout buffers that have been provided by the firmware. 
The attached graphics driver is secondary to this. But I'm struggling to 
find a simple sentence to express this. :/

Best regards
Thomas

> 
> For example, in the sysfb platform code and Kconfig symbol help text.
> 
>> Best regards
>> Thomas
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] video/aperture: fix typos

[Pavel Machek]

Hi!

> > Am 04.04.23 um 06:01 schrieb Sui Jingfeng:
> >>   EFI FB, VESA FB or VGA FB etc are belong to firmware based framebuffer
> >>   driver.
> >
> 
...
> I fixed that before applying, also removed the "are" in the sentence
> above, since it sounded off and repharsed subject line as "Fix typos
> in comments".

I seem to remember that 'all your bases are belong to us' is an old
meme, but that was probably not intentional here.

Best regards,
						Pavel

--