[PATCH 17/17] drm/mgag200: Replace VRAM helpers with SHMEM helpers

Thomas Zimmermann tzimmermann at suse.de
Mon May 4 12:45:41 UTC 2020


Hi

Am 04.05.20 um 14:29 schrieb Emil Velikov:
> Hi Thomas,
> 
> Just a couple of fly-by comments.
> 
> On Wed, 29 Apr 2020 at 15:33, Thomas Zimmermann <tzimmermann at suse.de> wrote:
> 
>> @@ -1618,21 +1640,13 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
>>         struct mga_device *mdev = to_mga_device(dev);
>>         struct drm_plane_state *state = plane->state;
>>         struct drm_framebuffer *fb = state->fb;
>> -       struct drm_gem_vram_object *gbo;
>> -       s64 gpu_addr;
>> +       struct drm_rect damage;
>>
>>         if (!fb)
>>                 return;
>>
>> -       gbo = drm_gem_vram_of_gem(fb->obj[0]);
>> -
>> -       gpu_addr = drm_gem_vram_offset(gbo);
>> -       if (drm_WARN_ON_ONCE(dev, gpu_addr < 0))
>> -               return; /* BUG: BO should have been pinned to VRAM. */
>> -
>> -       mgag200_set_format_regs(mdev, fb);
> This function seems to be missing from the handle_damage helper.
> 
> Is that intentional, something which should be squashed with another
> patch or it belongs in the helper?

Thanks for noticing. That line should not have been here at all.
Changing format registers might require an update to the PLL. And that
in turn requires a full modeset in _enable(). The enable function
already sets the format regs; this line can be removed.


> 
>> -       mgag200_set_startadd(mdev, (unsigned long)gpu_addr);
>> -       mgag200_set_offset(mdev, fb);
>> +       if (drm_atomic_helper_damage_merged(old_state, state, &damage))
>> +               mgag200_handle_damage(mdev, fb, &damage);
>>  }
> 
> 
> 
>>  int mgag200_mm_init(struct mga_device *mdev)
>>  {
> 
>> +       arch_io_reserve_memtype_wc(pci_resource_start(pdev, 0),
>> +                                  pci_resource_len(pdev, 0));
>>
>> -       arch_io_reserve_memtype_wc(pci_resource_start(dev->pdev, 0),
>> -                                  pci_resource_len(dev->pdev, 0));
>> +       mdev->fb_mtrr = arch_phys_wc_add(pci_resource_start(pdev, 0),
>> +                                        pci_resource_len(pdev, 0));
>>
>> -       mdev->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0),
>> -                                        pci_resource_len(dev->pdev, 0));
> 
> Some spurious s/dev->pdev/pdev/g changes got in the way. Might as well
> do those separately...
> 
>> +       mdev->vram = ioremap(pci_resource_start(pdev, 0),
>> +                            pci_resource_len(pdev, 0));
>> +       if (!mdev->vram) {
>> +               ret = -ENOMEM;
>> +               goto err_arch_phys_wc_del;
>> +       }
>>
>>         mdev->vram_fb_available = mdev->mc.vram_size;
>>
>>         return 0;
>> +
>> +err_arch_phys_wc_del:
>> +       arch_phys_wc_del(mdev->fb_mtrr);
>> +       arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
>> +                               pci_resource_len(dev->pdev, 0));
> ... and consistently?

Good points.

Best regards
Thomas

> 
> HTH
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200504/ab305a33/attachment.sig>


More information about the dri-devel mailing list