[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 at 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);
>>>       }
>>>   }
>>
>