Re: [PATCH 17/17] drm/mgag200: Replace VRAM helpers with SHMEM helpers
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 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, ))
>> + mgag200_handle_damage(mdev, fb, );
>> }
>
>
>
>> 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
signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 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, ))
> + mgag200_handle_damage(mdev, fb, );
> }
> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 17/17] drm/mgag200: Replace VRAM helpers with SHMEM helpers
The VRAM helpers managed the framebuffer memory for mgag200. This came
with several problems, as some MGA device require the scanout address
to be located at VRAM offset 0. It's incompatible with the page-flip
semantics of DRM's atomic modesettting. With atomic modesetting, old and
new framebuffers have to be located in VRAM at the same time. So at least
one of them has to reside at a non-0 offset.
This patch replaces VRAM helpers with SHMEM helpers. GEM SHMEM buffers
reside in system memory, and are shadow-copied into VRAM during page
flips. The shadow copy always starts at VRAM offset 0.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/Kconfig| 4 +-
drivers/gpu/drm/mgag200/mgag200_drv.c | 49 +
drivers/gpu/drm/mgag200/mgag200_drv.h | 5 ++-
drivers/gpu/drm/mgag200/mgag200_mode.c | 59 +++---
drivers/gpu/drm/mgag200/mgag200_ttm.c | 35 ---
5 files changed, 60 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
index d60aa4b9ccd47..93be766715c9b 100644
--- a/drivers/gpu/drm/mgag200/Kconfig
+++ b/drivers/gpu/drm/mgag200/Kconfig
@@ -2,10 +2,8 @@
config DRM_MGAG200
tristate "Kernel modesetting driver for MGA G200 server engines"
depends on DRM && PCI && MMU
+ select DRM_GEM_SHMEM_HELPER
select DRM_KMS_HELPER
- select DRM_VRAM_HELPER
- select DRM_TTM
- select DRM_TTM_HELPER
help
This is a KMS driver for the MGA G200 server chips, it
does not support the original MGA G200 or any of the desktop
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index b1272165621ed..00ddea7d7d270 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -22,15 +22,11 @@
* which then performs further device association and calls our graphics init
* functions
*/
-int mgag200_modeset = -1;
+int mgag200_modeset = -1;
MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
module_param_named(modeset, mgag200_modeset, int, 0400);
-int mgag200_hw_bug_no_startadd = -1;
-MODULE_PARM_DESC(modeset, "HW does not interpret scanout-buffer start address
correctly");
-module_param_named(hw_bug_no_startadd, mgag200_hw_bug_no_startadd, int, 0400);
-
static struct drm_driver driver;
static const struct pci_device_id pciidlist[] = {
@@ -101,44 +97,6 @@ static void mga_pci_remove(struct pci_dev *pdev)
DEFINE_DRM_GEM_FOPS(mgag200_driver_fops);
-static bool mgag200_pin_bo_at_0(const struct mga_device *mdev)
-{
- if (mgag200_hw_bug_no_startadd > 0) {
- DRM_WARN_ONCE("Option hw_bug_no_startradd is enabled. Please "
- "report the output of 'lspci -vvnn' to "
- " if this "
- "option is required to make mgag200 work "
- "correctly on your system.\n");
- return true;
- } else if (!mgag200_hw_bug_no_startadd) {
- return false;
- }
- return mdev->flags & MGAG200_FLAG_HW_BUG_NO_STARTADD;
-}
-
-int mgag200_driver_dumb_create(struct drm_file *file,
- struct drm_device *dev,
- struct drm_mode_create_dumb *args)
-{
- struct mga_device *mdev = dev->dev_private;
- unsigned long pg_align;
-
- if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
- return -EINVAL;
-
- pg_align = 0ul;
-
- /*
-* Aligning scanout buffers to the size of the video ram forces
-* placement at offset 0. Works around a bug where HW does not
-* respect 'startadd' field.
-*/
- if (mgag200_pin_bo_at_0(mdev))
- pg_align = PFN_UP(mdev->mc.vram_size);
-
- return drm_gem_vram_fill_create_dumb(file, dev, pg_align, 0, args);
-}
-
static struct drm_driver driver = {
.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
.fops = _driver_fops,
@@ -148,10 +106,7 @@ static struct drm_driver driver = {
.major = DRIVER_MAJOR,
.minor = DRIVER_MINOR,
.patchlevel = DRIVER_PATCHLEVEL,
- .debugfs_init = drm_vram_mm_debugfs_init,
- .dumb_create = mgag200_driver_dumb_create,
- .dumb_map_offset = drm_gem_vram_driver_dumb_mmap_offset,
- .gem_prime_mmap = drm_gem_prime_mmap,
+ DRM_GEM_SHMEM_DRIVER_OPS,
};
static struct pci_driver mgag200_pci_driver = {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 2e407508714c8..8c37177b8fa0c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -18,7 +18,7 @@
#include
#include
#include
-#include
+#include
#include
#include "mgag200_reg.h"
@@ -164,7 +164,8 @@ struct mga_device {
struct mga_mc mc;
- size_t
