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