[PATCH v2] drm/ast: Fix start address computation

[PATCH v2] drm/ast: Fix start address computation

[Jocelyn Falempe]

During the driver conversion to shmem, the start address for the
scanout buffer was set to the base PCI address.
In most cases it works because only the lower 24bits are used, and
due to alignment it was almost always 0.
But on some unlucky hardware, it's not the case, and some unitilized
memory is displayed on the BMC.
With shmem, the primary plane is always at offset 0 in GPU memory.

Tested on a sr645 affected by this bug.

Fixes: f2fa5a99ca81 ("drm/ast: Convert ast to SHMEM")
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/ast/ast_mode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index c7443317c747..54deb29bfeb3 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -681,7 +681,8 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
 		ast_set_offset_reg(ast, fb);
 	if (!old_fb) {
-		ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
+		/* with shmem, the primary plane is always at offset 0 */
+		ast_set_start_address_crt1(ast, 0);
 		ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
 	}
 }
-- 
2.39.1

Re: [PATCH v2] drm/ast: Fix start address computation

[Jammy Huang]

Hello,

The offset given to ast_set_start_address_crt1() should be offset in 
vram. It should 0 now as your patch.

I think it is better to correct it in ast_primary_plane_init() and 
ast_cursor_plane_init() as below.

--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -714,7 +714,7 @@ static int ast_primary_plane_init(struct ast_private 
*ast)
         struct ast_plane *ast_primary_plane = &ast->primary_plane;
         struct drm_plane *primary_plane = &ast_primary_plane->base;
         void __iomem *vaddr = ast->vram;
-       u64 offset = ast->vram_base;
+       u64 offset = 0;
         unsigned long cursor_size = roundup(AST_HWC_SIZE + 
AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
         unsigned long size = ast->vram_fb_available - cursor_size;
         int ret;
@@ -972,7 +972,7 @@ static int ast_cursor_plane_init(struct ast_private 
*ast)
                 return -ENOMEM;

         vaddr = ast->vram + ast->vram_fb_available - size;
-       offset = ast->vram_base + ast->vram_fb_available - size;
+       offset = st->vram_fb_available - size;

On 2023/2/9 下午 05:12, Jocelyn Falempe wrote:
> During the driver conversion to shmem, the start address for the
> scanout buffer was set to the base PCI address.
> In most cases it works because only the lower 24bits are used, and
> due to alignment it was almost always 0.
> But on some unlucky hardware, it's not the case, and some unitilized
> memory is displayed on the BMC.
> With shmem, the primary plane is always at offset 0 in GPU memory.
>
> Tested on a sr645 affected by this bug.
>
> Fixes: f2fa5a99ca81 ("drm/ast: Convert ast to SHMEM")
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/ast/ast_mode.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index c7443317c747..54deb29bfeb3 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -681,7 +681,8 @@ static void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
>   		ast_set_offset_reg(ast, fb);
>   	if (!old_fb) {
> -		ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
> +		/* with shmem, the primary plane is always at offset 0 */
> +		ast_set_start_address_crt1(ast, 0);
>   		ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>   	}
>   }

-- 
Best Regards
Jammy

Re: [PATCH v2] drm/ast: Fix start address computation

[Jocelyn Falempe]

On 09/02/2023 10:22, Jammy Huang wrote:
> Hello,
> 
> The offset given to ast_set_start_address_crt1() should be offset in 
> vram. It should 0 now as your patch.
> 
> I think it is better to correct it in ast_primary_plane_init() and 
> ast_cursor_plane_init() as below.

ok, thanks for the review, I will send a v3 with this suggestion.

-- 

Jocelyn

> 
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -714,7 +714,7 @@ static int ast_primary_plane_init(struct ast_private 
> *ast)
>          struct ast_plane *ast_primary_plane = &ast->primary_plane;
>          struct drm_plane *primary_plane = &ast_primary_plane->base;
>          void __iomem *vaddr = ast->vram;
> -       u64 offset = ast->vram_base;
> +       u64 offset = 0;
>          unsigned long cursor_size = roundup(AST_HWC_SIZE + 
> AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
>          unsigned long size = ast->vram_fb_available - cursor_size;
>          int ret;
> @@ -972,7 +972,7 @@ static int ast_cursor_plane_init(struct ast_private 
> *ast)
>                  return -ENOMEM;
> 
>          vaddr = ast->vram + ast->vram_fb_available - size;
> -       offset = ast->vram_base + ast->vram_fb_available - size;
> +       offset = st->vram_fb_available - size;
> 
> On 2023/2/9 下午 05:12, Jocelyn Falempe wrote:
>> During the driver conversion to shmem, the start address for the
>> scanout buffer was set to the base PCI address.
>> In most cases it works because only the lower 24bits are used, and
>> due to alignment it was almost always 0.
>> But on some unlucky hardware, it's not the case, and some unitilized
>> memory is displayed on the BMC.
>> With shmem, the primary plane is always at offset 0 in GPU memory.
>>
>> Tested on a sr645 affected by this bug.
>>
>> Fixes: f2fa5a99ca81 ("drm/ast: Convert ast to SHMEM")
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/ast/ast_mode.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>> b/drivers/gpu/drm/ast/ast_mode.c
>> index c7443317c747..54deb29bfeb3 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -681,7 +681,8 @@ static void 
>> ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>       if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
>>           ast_set_offset_reg(ast, fb);
>>       if (!old_fb) {
>> -        ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>> +        /* with shmem, the primary plane is always at offset 0 */
>> +        ast_set_start_address_crt1(ast, 0);
>>           ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>>       }
>>   }
> 

Re: [PATCH v2] drm/ast: Fix start address computation

[Thomas Zimmermann]

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

Hi

Am 09.02.23 um 10:22 schrieb Jammy Huang:
> Hello,
> 
> The offset given to ast_set_start_address_crt1() should be offset in 
> vram. It should 0 now as your patch.
> 
> I think it is better to correct it in ast_primary_plane_init() and 
> ast_cursor_plane_init() as below.

I was about to write the same. Thanks for the review.

> 
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -714,7 +714,7 @@ static int ast_primary_plane_init(struct ast_private 
> *ast)
>          struct ast_plane *ast_primary_plane = &ast->primary_plane;
>          struct drm_plane *primary_plane = &ast_primary_plane->base;
>          void __iomem *vaddr = ast->vram;
> -       u64 offset = ast->vram_base;
> +       u64 offset = 0;
>          unsigned long cursor_size = roundup(AST_HWC_SIZE + 
> AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
>          unsigned long size = ast->vram_fb_available - cursor_size;
>          int ret;
> @@ -972,7 +972,7 @@ static int ast_cursor_plane_init(struct ast_private 
> *ast)
>                  return -ENOMEM;
> 
>          vaddr = ast->vram + ast->vram_fb_available - size;
> -       offset = ast->vram_base + ast->vram_fb_available - size;
> +       offset = st->vram_fb_available - size;

This is confusing me, because in my tests I still saw a cursor, even 
though the address is currently wrong.

Best regard
Thomas

> 
> On 2023/2/9 下午 05:12, Jocelyn Falempe wrote:
>> During the driver conversion to shmem, the start address for the
>> scanout buffer was set to the base PCI address.
>> In most cases it works because only the lower 24bits are used, and
>> due to alignment it was almost always 0.
>> But on some unlucky hardware, it's not the case, and some unitilized
>> memory is displayed on the BMC.
>> With shmem, the primary plane is always at offset 0 in GPU memory.
>>
>> Tested on a sr645 affected by this bug.
>>
>> Fixes: f2fa5a99ca81 ("drm/ast: Convert ast to SHMEM")
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/ast/ast_mode.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>> b/drivers/gpu/drm/ast/ast_mode.c
>> index c7443317c747..54deb29bfeb3 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -681,7 +681,8 @@ static void 
>> ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>       if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
>>           ast_set_offset_reg(ast, fb);
>>       if (!old_fb) {
>> -        ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>> +        /* with shmem, the primary plane is always at offset 0 */
>> +        ast_set_start_address_crt1(ast, 0);
>>           ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>>       }
>>   }
> 

-- 
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 v2] drm/ast: Fix start address computation

[Jocelyn Falempe]

On 09/02/2023 10:35, Thomas Zimmermann wrote:
> Hi
> 
> Am 09.02.23 um 10:22 schrieb Jammy Huang:
>> Hello,
>>
>> The offset given to ast_set_start_address_crt1() should be offset in 
>> vram. It should 0 now as your patch.
>>
>> I think it is better to correct it in ast_primary_plane_init() and 
>> ast_cursor_plane_init() as below.
> 
> I was about to write the same. Thanks for the review.
> 
>>
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -714,7 +714,7 @@ static int ast_primary_plane_init(struct 
>> ast_private *ast)
>>          struct ast_plane *ast_primary_plane = &ast->primary_plane;
>>          struct drm_plane *primary_plane = &ast_primary_plane->base;
>>          void __iomem *vaddr = ast->vram;
>> -       u64 offset = ast->vram_base;
>> +       u64 offset = 0;
>>          unsigned long cursor_size = roundup(AST_HWC_SIZE + 
>> AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
>>          unsigned long size = ast->vram_fb_available - cursor_size;
>>          int ret;
>> @@ -972,7 +972,7 @@ static int ast_cursor_plane_init(struct 
>> ast_private *ast)
>>                  return -ENOMEM;
>>
>>          vaddr = ast->vram + ast->vram_fb_available - size;
>> -       offset = ast->vram_base + ast->vram_fb_available - size;
>> +       offset = st->vram_fb_available - size;
> 
> This is confusing me, because in my tests I still saw a cursor, even 
> though the address is currently wrong.

I think it's because the PCI base address has its 26 bits lower part set 
to 0. So in most hardware it will works.
you can see in ast_set_cursor_base() that only the lower 26 bits are used.

-- 

Jocelyn

> 
> Best regard
> Thomas
> 
>>
>> On 2023/2/9 下午 05:12, Jocelyn Falempe wrote:
>>> During the driver conversion to shmem, the start address for the
>>> scanout buffer was set to the base PCI address.
>>> In most cases it works because only the lower 24bits are used, and
>>> due to alignment it was almost always 0.
>>> But on some unlucky hardware, it's not the case, and some unitilized
>>> memory is displayed on the BMC.
>>> With shmem, the primary plane is always at offset 0 in GPU memory.
>>>
>>> Tested on a sr645 affected by this bug.
>>>
>>> Fixes: f2fa5a99ca81 ("drm/ast: Convert ast to SHMEM")
>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>>> ---
>>>   drivers/gpu/drm/ast/ast_mode.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>>> b/drivers/gpu/drm/ast/ast_mode.c
>>> index c7443317c747..54deb29bfeb3 100644
>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>> @@ -681,7 +681,8 @@ static void 
>>> ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>>       if (!old_fb || old_fb->pitches[0] != fb->pitches[0])
>>>           ast_set_offset_reg(ast, fb);
>>>       if (!old_fb) {
>>> -        ast_set_start_address_crt1(ast, (u32)ast_plane->offset);
>>> +        /* with shmem, the primary plane is always at offset 0 */
>>> +        ast_set_start_address_crt1(ast, 0);
>>>           ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
>>>       }
>>>   }
>>
>