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

Hi Thomas,

Just a couple of fly-by comments.

On Wed, 29 Apr 2020 at 15:33, Thomas Zimmermann tzimmermann@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?

•   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?

HTH Emil