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

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

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