[PATCH] drm/ast: Do not enable PCI resources multiple times

[PATCH] drm/ast: Do not enable PCI resources multiple times

[Thomas Zimmermann]

Remove ast_init_pci_config() as the ast driver already enables the PCI
resources by calling pcim_enable_device().

Suggested-by: Sui Jingfeng <suijingfeng@loongson.cn>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 8bfbdfd86d77..dae365ed3969 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -35,23 +35,6 @@
 
 #include "ast_drv.h"
 
-static int ast_init_pci_config(struct pci_dev *pdev)
-{
-	int err;
-	u16 pcis04;
-
-	err = pci_read_config_word(pdev, PCI_COMMAND, &pcis04);
-	if (err)
-		goto out;
-
-	pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-
-	err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
-
-out:
-	return pcibios_err_to_errno(err);
-}
-
 static bool ast_is_vga_enabled(struct drm_device *dev)
 {
 	struct ast_device *ast = to_ast_device(dev);
@@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 			return ERR_PTR(-EIO);
 	}
 
-	ret = ast_init_pci_config(pdev);
-	if (ret)
-		return ERR_PTR(ret);
-
 	if (!ast_is_vga_enabled(dev)) {
 		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
 		need_post = true;
-- 
2.41.0

Re: [PATCH] drm/ast: Do not enable PCI resources multiple times

[Jocelyn Falempe]

On 12/07/2023 15:08, Thomas Zimmermann wrote:
> Remove ast_init_pci_config() as the ast driver already enables the PCI
> resources by calling pcim_enable_device().
> 
> Suggested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_main.c | 21 ---------------------
>   1 file changed, 21 deletions(-)
> 

looks good to me.
indeed pcim_enable_device() call pci_enable_resources() which set
PCI_COMMAND_MEMORY and PCI_COMMAND_IO flags.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>


Best regards,

-- 

Jocelyn


> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 8bfbdfd86d77..dae365ed3969 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -35,23 +35,6 @@
>   
>   #include "ast_drv.h"
>   
> -static int ast_init_pci_config(struct pci_dev *pdev)
> -{
> -	int err;
> -	u16 pcis04;
> -
> -	err = pci_read_config_word(pdev, PCI_COMMAND, &pcis04);
> -	if (err)
> -		goto out;
> -
> -	pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
> -
> -	err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
> -
> -out:
> -	return pcibios_err_to_errno(err);
> -}
> -
>   static bool ast_is_vga_enabled(struct drm_device *dev)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
> @@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   			return ERR_PTR(-EIO);
>   	}
>   
> -	ret = ast_init_pci_config(pdev);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>   	if (!ast_is_vga_enabled(dev)) {
>   		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
>   		need_post = true;

Re: drm/ast: Do not enable PCI resources multiple times

[suijingfeng]

Hi,


I have tested this patch on my x86-64(i3-8100, H110 D4L board) + ast2400 
discrete BMC card just now,

drm/ast still works on normal case.


But  originally this function is called in ast_post_gpu() function.

ast_post_gpu() doesn't happen on my test case.


I know something about the POST (Power On Self Test),

In the past, my mips and loongarch have difficulty to run x86 VBIOS code.

So part of my radeon and amdgpu gpu card will just hang(stall) at the 
bios/vbios boot stage.

When we debugging, we hasty solve this problem by disabling (comment out)

the VBIOS part initialization code of the BIOS.

The results is that the screen will not light-up during the boot stage.

We relay the kernel-space drivers (drm/radeon and drm/amdgpu) to 
initialize the GPU.

In this case, (drm/radeon and drm/amdgpu) driver log(dmesg) will print : 
"need POST ...".


So, from my past experience, I think the "need POST" will only happens

when there no firmware support for a specific machine + BMC combination.

But this is not a normal case.


So, I think we probably could try this, if there no objections from 
experienced reviewers.


On 2023/7/12 21:08, Thomas Zimmermann wrote:
> Remove ast_init_pci_config() as the ast driver already enables the PCI
> resources by calling pcim_enable_device().
>
> Suggested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>


Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>


> ---
>   drivers/gpu/drm/ast/ast_main.c | 21 ---------------------
>   1 file changed, 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 8bfbdfd86d77..dae365ed3969 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -35,23 +35,6 @@
>   
>   #include "ast_drv.h"
>   
> -static int ast_init_pci_config(struct pci_dev *pdev)
> -{
> -	int err;
> -	u16 pcis04;
> -
> -	err = pci_read_config_word(pdev, PCI_COMMAND, &pcis04);
> -	if (err)
> -		goto out;
> -
> -	pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
> -
> -	err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
> -
> -out:
> -	return pcibios_err_to_errno(err);
> -}
> -
>   static bool ast_is_vga_enabled(struct drm_device *dev)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
> @@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   			return ERR_PTR(-EIO);
>   	}
>   
> -	ret = ast_init_pci_config(pdev);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>   	if (!ast_is_vga_enabled(dev)) {
>   		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
>   		need_post = true;

Re: drm/ast: Do not enable PCI resources multiple times

[suijingfeng]

Hi,


Actually,  I'm only a little bit worry about the ast_pm_thaw() code path.

|- ast_pm_thaw()

|-- ast_drm_thaw()

|--- ast_post_gpu()


Except this, all other code path have pci_enable_device() or 
pcim_enable_device() called.

So, this patch seems OK.


On 2023/7/12 21:08, Thomas Zimmermann wrote:
> Remove ast_init_pci_config() as the ast driver already enables the PCI
> resources by calling pcim_enable_device().
>
> Suggested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/ast/ast_main.c | 21 ---------------------
>   1 file changed, 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 8bfbdfd86d77..dae365ed3969 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -35,23 +35,6 @@
>   
>   #include "ast_drv.h"
>   
> -static int ast_init_pci_config(struct pci_dev *pdev)
> -{
> -	int err;
> -	u16 pcis04;
> -
> -	err = pci_read_config_word(pdev, PCI_COMMAND, &pcis04);
> -	if (err)
> -		goto out;
> -
> -	pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
> -
> -	err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
> -
> -out:
> -	return pcibios_err_to_errno(err);
> -}
> -
>   static bool ast_is_vga_enabled(struct drm_device *dev)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
> @@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   			return ERR_PTR(-EIO);
>   	}
>   
> -	ret = ast_init_pci_config(pdev);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>   	if (!ast_is_vga_enabled(dev)) {
>   		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
>   		need_post = true;

Re: drm/ast: Do not enable PCI resources multiple times

[Thomas Zimmermann]

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

Hi

Am 18.07.23 um 07:40 schrieb suijingfeng:
> Hi,
> 
> 
> Actually,  I'm only a little bit worry about the ast_pm_thaw() code path.
> 
> |- ast_pm_thaw()
> 
> |-- ast_drm_thaw()
> 
> |--- ast_post_gpu()

I'm not quite sure what mean here, because the post-GPU code is not 
involved in this patch. All this patch does is to remove duplicated code.

Is there a bug in the post-GPU handling?

Best regards
Thomas

> 
> 
> Except this, all other code path have pci_enable_device() or 
> pcim_enable_device() called.
> 
> So, this patch seems OK.
> 
> 
> On 2023/7/12 21:08, Thomas Zimmermann wrote:
>> Remove ast_init_pci_config() as the ast driver already enables the PCI
>> resources by calling pcim_enable_device().
>>
>> Suggested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/ast/ast_main.c | 21 ---------------------
>>   1 file changed, 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>> b/drivers/gpu/drm/ast/ast_main.c
>> index 8bfbdfd86d77..dae365ed3969 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -35,23 +35,6 @@
>>   #include "ast_drv.h"
>> -static int ast_init_pci_config(struct pci_dev *pdev)
>> -{
>> -    int err;
>> -    u16 pcis04;
>> -
>> -    err = pci_read_config_word(pdev, PCI_COMMAND, &pcis04);
>> -    if (err)
>> -        goto out;
>> -
>> -    pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
>> -
>> -    err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
>> -
>> -out:
>> -    return pcibios_err_to_errno(err);
>> -}
>> -
>>   static bool ast_is_vga_enabled(struct drm_device *dev)
>>   {
>>       struct ast_device *ast = to_ast_device(dev);
>> @@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const struct 
>> drm_driver *drv,
>>               return ERR_PTR(-EIO);
>>       }
>> -    ret = ast_init_pci_config(pdev);
>> -    if (ret)
>> -        return ERR_PTR(ret);
>> -
>>       if (!ast_is_vga_enabled(dev)) {
>>           drm_info(dev, "VGA not enabled on entry, requesting chip 
>> POST\n");
>>           need_post = true;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: drm/ast: Do not enable PCI resources multiple times

[suijingfeng]

Hi,

On 2023/7/25 02:34, Thomas Zimmermann wrote:
> Hi
>
> Am 18.07.23 um 07:40 schrieb suijingfeng:
>> Hi,
>>
>>
>> Actually,  I'm only a little bit worry about the ast_pm_thaw() code 
>> path.
>>
>> |- ast_pm_thaw()
>>
>> |-- ast_drm_thaw()
>>
>> |--- ast_post_gpu()
>

> I'm not quite sure what mean here, because the post-GPU code is not 
> involved in this patch.


Ah, I'm get confused with a previous patch.

Previously, the drm/ast driver call ast_init_pci_config() function in 
ast_post_gpu().

I also don't know why it need to do so.

Ok then, just remove it.


> All this patch does is to remove duplicated code.
>

Yes, this patch has nothing to do with the ast_post_gpu() function.


> Is there a bug in the post-GPU handling?
>

No, I'm not find one so far.


> Best regards
> Thomas
>
>>
>>
>> Except this, all other code path have pci_enable_device() or 
>> pcim_enable_device() called.
>>
>> So, this patch seems OK.
>>
>>
>> On 2023/7/12 21:08, Thomas Zimmermann wrote:
>>> Remove ast_init_pci_config() as the ast driver already enables the PCI
>>> resources by calling pcim_enable_device().
>>>
>>> Suggested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
>>> ---
>>>   drivers/gpu/drm/ast/ast_main.c | 21 ---------------------
>>>   1 file changed, 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>>> b/drivers/gpu/drm/ast/ast_main.c
>>> index 8bfbdfd86d77..dae365ed3969 100644
>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>> @@ -35,23 +35,6 @@
>>>   #include "ast_drv.h"
>>> -static int ast_init_pci_config(struct pci_dev *pdev)
>>> -{
>>> -    int err;
>>> -    u16 pcis04;
>>> -
>>> -    err = pci_read_config_word(pdev, PCI_COMMAND, &pcis04);
>>> -    if (err)
>>> -        goto out;
>>> -
>>> -    pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
>>> -
>>> -    err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
>>> -
>>> -out:
>>> -    return pcibios_err_to_errno(err);
>>> -}
>>> -
>>>   static bool ast_is_vga_enabled(struct drm_device *dev)
>>>   {
>>>       struct ast_device *ast = to_ast_device(dev);
>>> @@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const 
>>> struct drm_driver *drv,
>>>               return ERR_PTR(-EIO);
>>>       }
>>> -    ret = ast_init_pci_config(pdev);
>>> -    if (ret)
>>> -        return ERR_PTR(ret);
>>> -
>>>       if (!ast_is_vga_enabled(dev)) {
>>>           drm_info(dev, "VGA not enabled on entry, requesting chip 
>>> POST\n");
>>>           need_post = true;
>>
>

Re: drm/ast: Do not enable PCI resources multiple times

[Thomas Zimmermann]

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

Hi

Am 25.07.23 um 03:12 schrieb suijingfeng:
> Hi,
> 
> On 2023/7/25 02:34, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 18.07.23 um 07:40 schrieb suijingfeng:
>>> Hi,
>>>
>>>
>>> Actually,  I'm only a little bit worry about the ast_pm_thaw() code 
>>> path.
>>>
>>> |- ast_pm_thaw()
>>>
>>> |-- ast_drm_thaw()
>>>
>>> |--- ast_post_gpu()
>>
> 
>> I'm not quite sure what mean here, because the post-GPU code is not 
>> involved in this patch.
> 
> 
> Ah, I'm get confused with a previous patch.

Ok, no problem. I've now merged that patch.

Best regards
Thomas

> 
> Previously, the drm/ast driver call ast_init_pci_config() function in 
> ast_post_gpu().
> 
> I also don't know why it need to do so.
> 
> Ok then, just remove it.
> 
> 
>> All this patch does is to remove duplicated code.
>>
> 
> Yes, this patch has nothing to do with the ast_post_gpu() function.
> 
> 
>> Is there a bug in the post-GPU handling?
>>
> 
> No, I'm not find one so far.
> 
> 
>> Best regards
>> Thomas
>>
>>>
>>>
>>> Except this, all other code path have pci_enable_device() or 
>>> pcim_enable_device() called.
>>>
>>> So, this patch seems OK.
>>>
>>>
>>> On 2023/7/12 21:08, Thomas Zimmermann wrote:
>>>> Remove ast_init_pci_config() as the ast driver already enables the PCI
>>>> resources by calling pcim_enable_device().
>>>>
>>>> Suggested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
>>>> ---
>>>>   drivers/gpu/drm/ast/ast_main.c | 21 ---------------------
>>>>   1 file changed, 21 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>>>> b/drivers/gpu/drm/ast/ast_main.c
>>>> index 8bfbdfd86d77..dae365ed3969 100644
>>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>>> @@ -35,23 +35,6 @@
>>>>   #include "ast_drv.h"
>>>> -static int ast_init_pci_config(struct pci_dev *pdev)
>>>> -{
>>>> -    int err;
>>>> -    u16 pcis04;
>>>> -
>>>> -    err = pci_read_config_word(pdev, PCI_COMMAND, &pcis04);
>>>> -    if (err)
>>>> -        goto out;
>>>> -
>>>> -    pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
>>>> -
>>>> -    err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
>>>> -
>>>> -out:
>>>> -    return pcibios_err_to_errno(err);
>>>> -}
>>>> -
>>>>   static bool ast_is_vga_enabled(struct drm_device *dev)
>>>>   {
>>>>       struct ast_device *ast = to_ast_device(dev);
>>>> @@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const 
>>>> struct drm_driver *drv,
>>>>               return ERR_PTR(-EIO);
>>>>       }
>>>> -    ret = ast_init_pci_config(pdev);
>>>> -    if (ret)
>>>> -        return ERR_PTR(ret);
>>>> -
>>>>       if (!ast_is_vga_enabled(dev)) {
>>>>           drm_info(dev, "VGA not enabled on entry, requesting chip 
>>>> POST\n");
>>>>           need_post = true;
>>>
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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