[REGRESSION] VGA output with AST 2600 graphics.

Commit Message

Jocelyn Falempe June 1, 2022, 9:33 a.m. UTC

Hi,

I've found a regression in the ast driver, for AST2600 hardware.

before the upstream commit f9bd00e0ea9d
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=f9bd00e0ea9d9b04140aa969a9a13ad3597a1e4e

The ast driver handled AST 2600 chip like an AST 2500.

After this commit, it uses some default values, more like the older AST 
chip.

There are a lot of places in the driver like this:
https://elixir.bootlin.com/linux/v5.18.1/source/drivers/gpu/drm/ast/ast_post.c#L82
where it checks for (AST2300 || AST2400 || AST2500) but not for AST2600.

This makes the VGA output, to be blurred and flickered with whites lines 
on AST2600.

The issue is present since v5.11

For v5.11~v5.17 I propose a simple workaround (as there are no other 
reference to AST2600 in the driver):
  		ast->chip = AST2500;

starting from v5.18, there is another reference to AST2600 in the code
https://elixir.bootlin.com/linux/v5.18/source/drivers/gpu/drm/ast/ast_main.c#L212

So I think someone with good aspeed knowledge should review all 
locations where there is a test for AST2500, and figure out what should 
be done for AST2600

Thanks,

Comments

Thomas Zimmermann June 1, 2022, 10:33 a.m. UTC | #1

Hi Jocelyn,

thanks for reporting this bug.

Am 01.06.22 um 11:33 schrieb Jocelyn Falempe:
> Hi,
> 
> I've found a regression in the ast driver, for AST2600 hardware.
> 
> before the upstream commit f9bd00e0ea9d
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=f9bd00e0ea9d9b04140aa969a9a13ad3597a1e4e 
> 
> 
> The ast driver handled AST 2600 chip like an AST 2500.
> 
> After this commit, it uses some default values, more like the older AST 
> chip.
> 
> There are a lot of places in the driver like this:
> https://elixir.bootlin.com/linux/v5.18.1/source/drivers/gpu/drm/ast/ast_post.c#L82 
> 
> where it checks for (AST2300 || AST2400 || AST2500) but not for AST2600.
> 
> This makes the VGA output, to be blurred and flickered with whites lines 
> on AST2600.
> 
> The issue is present since v5.11
> 
> For v5.11~v5.17 I propose a simple workaround (as there are no other 
> reference to AST2600 in the driver):
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -146,7 +146,8 @@ static int ast_detect_chip(struct drm_device *dev, 
> bool *need_post)
> 
>       /* Identify chipset */
>       if (pdev->revision >= 0x50) {
> -        ast->chip = AST2600;
> +        /* Workaround to use the same codepath for AST2600 */
> +        ast->chip = AST2500;

The whole handling of different models in this driver is broken by 
design and needs to be replaced.  I don't have much of the affected 
hardware, so such things are going slowly. :(

For an intermediate fix, it would be better to change all tests for 
AST2500 to include AST2600 as well. There aren't too many IIRC.

Best regards
Thomas

>           drm_info(dev, "AST 2600 detected\n");
>       } else if (pdev->revision >= 0x40) {
>           ast->chip = AST2500;
> 
> starting from v5.18, there is another reference to AST2600 in the code
> https://elixir.bootlin.com/linux/v5.18/source/drivers/gpu/drm/ast/ast_main.c#L212 
> 
> 
> So I think someone with good aspeed knowledge should review all 
> locations where there is a test for AST2500, and figure out what should 
> be done for AST2600
> 
> Thanks,
>

Jocelyn Falempe June 1, 2022, 12:29 p.m. UTC | #2

On 01/06/2022 12:33, Thomas Zimmermann wrote:
> Hi Jocelyn,
> 
> thanks for reporting this bug.
> 
> Am 01.06.22 um 11:33 schrieb Jocelyn Falempe:
>> Hi,
>>
>> I've found a regression in the ast driver, for AST2600 hardware.
>>
>> before the upstream commit f9bd00e0ea9d
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=f9bd00e0ea9d9b04140aa969a9a13ad3597a1e4e 
>>
>>
>> The ast driver handled AST 2600 chip like an AST 2500.
>>
>> After this commit, it uses some default values, more like the older 
>> AST chip.
>>
>> There are a lot of places in the driver like this:
>> https://elixir.bootlin.com/linux/v5.18.1/source/drivers/gpu/drm/ast/ast_post.c#L82 
>>
>> where it checks for (AST2300 || AST2400 || AST2500) but not for AST2600.
>>
>> This makes the VGA output, to be blurred and flickered with whites 
>> lines on AST2600.
>>
>> The issue is present since v5.11
>>
>> For v5.11~v5.17 I propose a simple workaround (as there are no other 
>> reference to AST2600 in the driver):
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -146,7 +146,8 @@ static int ast_detect_chip(struct drm_device *dev, 
>> bool *need_post)
>>
>>       /* Identify chipset */
>>       if (pdev->revision >= 0x50) {
>> -        ast->chip = AST2600;
>> +        /* Workaround to use the same codepath for AST2600 */
>> +        ast->chip = AST2500;
> 
> The whole handling of different models in this driver is broken by 
> design and needs to be replaced.  I don't have much of the affected 
> hardware, so such things are going slowly. :(
> 
> For an intermediate fix, it would be better to change all tests for 
> AST2500 to include AST2600 as well. There aren't too many IIRC.

I feel a bit uncomfortable doing this, because I don't know if this 
settings are good for AST2600 or not. I just know that AST2500 settings 
are better than the "default".

Also it may not apply cleanly up to v5.11

I will do a test patch and see what it gives.

Another solution would be to just revert f9bd00e0ea9d for v5.11 to v5.17 ?

Best regards,

Thorsten Leemhuis June 7, 2022, 11:02 a.m. UTC | #3

Hi, this is your Linux kernel regression tracker.

On 01.06.22 14:29, Jocelyn Falempe wrote:
> On 01/06/2022 12:33, Thomas Zimmermann wrote:
>> Am 01.06.22 um 11:33 schrieb Jocelyn Falempe:
>>>
>>> I've found a regression in the ast driver, for AST2600 hardware.
>>>
>>> before the upstream commit f9bd00e0ea9d
>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=f9bd00e0ea9d9b04140aa969a9a13ad3597a1e4e
>>>
>>>
>>> The ast driver handled AST 2600 chip like an AST 2500.
>>>
>>> After this commit, it uses some default values, more like the older
>>> AST chip.
>>>
>>> There are a lot of places in the driver like this:
>>> https://elixir.bootlin.com/linux/v5.18.1/source/drivers/gpu/drm/ast/ast_post.c#L82
>>>
>>> where it checks for (AST2300 || AST2400 || AST2500) but not for AST2600.
>>>
>>> This makes the VGA output, to be blurred and flickered with whites
>>> lines on AST2600.
>>>
>>> The issue is present since v5.11
>>>
>>> For v5.11~v5.17 I propose a simple workaround (as there are no other
>>> reference to AST2600 in the driver):
>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>> @@ -146,7 +146,8 @@ static int ast_detect_chip(struct drm_device
>>> *dev, bool *need_post)
>>>
>>>       /* Identify chipset */
>>>       if (pdev->revision >= 0x50) {
>>> -        ast->chip = AST2600;
>>> +        /* Workaround to use the same codepath for AST2600 */
>>> +        ast->chip = AST2500;
>>
>> The whole handling of different models in this driver is broken by
>> design and needs to be replaced.  I don't have much of the affected
>> hardware, so such things are going slowly. :(
>>
>> For an intermediate fix, it would be better to change all tests for
>> AST2500 to include AST2600 as well. There aren't too many IIRC.
>
> I feel a bit uncomfortable doing this, because I don't know if this
> settings are good for AST2600 or not. I just know that AST2500 settings
> are better than the "default".

KuoHsiang Chou, you wrote the commit causing this regression. Could you
maybe take care of the idea Thomas outlined to get this fixed relative
quickly? Or do you have a better idea?

> Also it may not apply cleanly up to v5.11

FWIW, 5.11 is EOL anyway.

> I will do a test patch and see what it gives.
> 
> Another solution would be to just revert f9bd00e0ea9d for v5.11 to v5.17 ?

That might cause a regression for users that depend on something
supported thx to this change. :-/

Ciao, Thorsten

Thomas Zimmermann June 7, 2022, 12:05 p.m. UTC | #4

Hi

Am 07.06.22 um 13:02 schrieb Thorsten Leemhuis:
> Hi, this is your Linux kernel regression tracker.
> 
> On 01.06.22 14:29, Jocelyn Falempe wrote:
>> On 01/06/2022 12:33, Thomas Zimmermann wrote:
>>> Am 01.06.22 um 11:33 schrieb Jocelyn Falempe:
>>>>
>>>> I've found a regression in the ast driver, for AST2600 hardware.
>>>>
>>>> before the upstream commit f9bd00e0ea9d
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=f9bd00e0ea9d9b04140aa969a9a13ad3597a1e4e
>>>>
>>>>
>>>> The ast driver handled AST 2600 chip like an AST 2500.
>>>>
>>>> After this commit, it uses some default values, more like the older
>>>> AST chip.
>>>>
>>>> There are a lot of places in the driver like this:
>>>> https://elixir.bootlin.com/linux/v5.18.1/source/drivers/gpu/drm/ast/ast_post.c#L82
>>>>
>>>> where it checks for (AST2300 || AST2400 || AST2500) but not for AST2600.
>>>>
>>>> This makes the VGA output, to be blurred and flickered with whites
>>>> lines on AST2600.
>>>>
>>>> The issue is present since v5.11
>>>>
>>>> For v5.11~v5.17 I propose a simple workaround (as there are no other
>>>> reference to AST2600 in the driver):
>>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>>> @@ -146,7 +146,8 @@ static int ast_detect_chip(struct drm_device
>>>> *dev, bool *need_post)
>>>>
>>>>        /* Identify chipset */
>>>>        if (pdev->revision >= 0x50) {
>>>> -        ast->chip = AST2600;
>>>> +        /* Workaround to use the same codepath for AST2600 */
>>>> +        ast->chip = AST2500;
>>>
>>> The whole handling of different models in this driver is broken by
>>> design and needs to be replaced.  I don't have much of the affected
>>> hardware, so such things are going slowly. :(
>>>
>>> For an intermediate fix, it would be better to change all tests for
>>> AST2500 to include AST2600 as well. There aren't too many IIRC.
>>
>> I feel a bit uncomfortable doing this, because I don't know if this
>> settings are good for AST2600 or not. I just know that AST2500 settings
>> are better than the "default".
> 
> KuoHsiang Chou, you wrote the commit causing this regression. Could you
> maybe take care of the idea Thomas outlined to get this fixed relative
> quickly? Or do you have a better idea?

Thanks for the reminder. I've sent out a patch for this problem. [1]

Best regards
Thomas

[1] 
https://lore.kernel.org/dri-devel/[e-mail]/T/#u

> 
>> Also it may not apply cleanly up to v5.11
> 
> FWIW, 5.11 is EOL anyway.
> 
>> I will do a test patch and see what it gives.
>>
>> Another solution would be to just revert f9bd00e0ea9d for v5.11 to v5.17 ?
> 
> That might cause a regression for users that depend on something
> supported thx to this change. :-/
> 
> Ciao, Thorsten

Patch

--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -146,7 +146,8 @@  static int ast_detect_chip(struct drm_device *dev, 
bool *need_post)

  	/* Identify chipset */
  	if (pdev->revision >= 0x50) {
-		ast->chip = AST2600;
+		/* Workaround to use the same codepath for AST2600 */
+		ast->chip = AST2500;
  		drm_info(dev, "AST 2600 detected\n");
  	} else if (pdev->revision >= 0x40) {