* [PATCH 0/4] drm/mgag200: Workaround HW bug with non-0 offset @ 2019-11-26 7:25 Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 1/4] drm/mgag200: Extract device type from flags Thomas Zimmermann ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Thomas Zimmermann @ 2019-11-26 7:25 UTC (permalink / raw) To: airlied, daniel, john.p.donnelly, kraxel, sam Cc: Thomas Zimmermann, dri-devel We found an MGA chip that does not interpret the scanout offset correctly. This patch works around the problem by placing all buffer objects at offset 0 on this system. There's a new module parameter 'hw_bug_no_startadd' that enables and disables the offset-0 placement. Default is auto-detection. Thomas Zimmermann (4): drm/mgag200: Extract device type from flags drm/mgag200: Store flags from PCI driver data in device structure drm/mgag200: Add workaround for HW that does not support 'startadd' drm/mgag200: Add module parameter to pin all buffers at offset 0 drivers/gpu/drm/mgag200/mgag200_drv.c | 44 +++++++++++++++++++++++++- drivers/gpu/drm/mgag200/mgag200_drv.h | 18 +++++++++++ drivers/gpu/drm/mgag200/mgag200_main.c | 3 +- 3 files changed, 63 insertions(+), 2 deletions(-) -- 2.23.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/4] drm/mgag200: Extract device type from flags 2019-11-26 7:25 [PATCH 0/4] drm/mgag200: Workaround HW bug with non-0 offset Thomas Zimmermann @ 2019-11-26 7:25 ` Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 2/4] drm/mgag200: Store flags from PCI driver data in device structure Thomas Zimmermann ` (2 subsequent siblings) 3 siblings, 0 replies; 14+ messages in thread From: Thomas Zimmermann @ 2019-11-26 7:25 UTC (permalink / raw) To: airlied, daniel, john.p.donnelly, kraxel, sam Cc: Thomas Zimmermann, dri-devel Adds a conversion function that extracts the device type from the PCI id-table flags. Allows for storing additional information in the other flag bits. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_drv.h | 7 +++++++ drivers/gpu/drm/mgag200/mgag200_main.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 0ea9a525e57d..976404634092 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -150,6 +150,8 @@ enum mga_type { G200_EW3, }; +#define MGAG200_TYPE_MASK (0x000000ff) + #define IS_G200_SE(mdev) (mdev->type == G200_SE_A || mdev->type == G200_SE_B) struct mga_device { @@ -181,6 +183,11 @@ struct mga_device { u32 unique_rev_id; }; +static inline enum mga_type +mgag200_type_from_driver_data(kernel_ulong_t driver_data) +{ + return (enum mga_type)(driver_data & MGAG200_TYPE_MASK); +} /* mgag200_mode.c */ int mgag200_modeset_init(struct mga_device *mdev); void mgag200_modeset_fini(struct mga_device *mdev); diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 5f74aabcd3df..517c5693ad69 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -94,7 +94,7 @@ static int mgag200_device_init(struct drm_device *dev, struct mga_device *mdev = dev->dev_private; int ret, option; - mdev->type = flags; + mdev->type = mgag200_type_from_driver_data(flags); /* Hardcode the number of CRTCs to 1 */ mdev->num_crtc = 1; -- 2.23.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/4] drm/mgag200: Store flags from PCI driver data in device structure 2019-11-26 7:25 [PATCH 0/4] drm/mgag200: Workaround HW bug with non-0 offset Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 1/4] drm/mgag200: Extract device type from flags Thomas Zimmermann @ 2019-11-26 7:25 ` Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 4/4] drm/mgag200: Add module parameter to pin all buffers at offset 0 Thomas Zimmermann 3 siblings, 0 replies; 14+ messages in thread From: Thomas Zimmermann @ 2019-11-26 7:25 UTC (permalink / raw) To: airlied, daniel, john.p.donnelly, kraxel, sam Cc: Thomas Zimmermann, dri-devel The flags field in struct mga_device has been unused so far. We now use it to store flag bits from the PCI driver. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_drv.h | 8 ++++++++ drivers/gpu/drm/mgag200/mgag200_main.c | 1 + 2 files changed, 9 insertions(+) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 976404634092..4b4f9ce74a84 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -151,6 +151,7 @@ enum mga_type { }; #define MGAG200_TYPE_MASK (0x000000ff) +#define MGAG200_FLAG_MASK (0x00ffff00) #define IS_G200_SE(mdev) (mdev->type == G200_SE_A || mdev->type == G200_SE_B) @@ -188,6 +189,13 @@ mgag200_type_from_driver_data(kernel_ulong_t driver_data) { return (enum mga_type)(driver_data & MGAG200_TYPE_MASK); } + +static inline unsigned long +mgag200_flags_from_driver_data(kernel_ulong_t driver_data) +{ + return driver_data & MGAG200_FLAG_MASK; +} + /* mgag200_mode.c */ int mgag200_modeset_init(struct mga_device *mdev); void mgag200_modeset_fini(struct mga_device *mdev); diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 517c5693ad69..e1bc5b0aa774 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -94,6 +94,7 @@ static int mgag200_device_init(struct drm_device *dev, struct mga_device *mdev = dev->dev_private; int ret, option; + mdev->flags = mgag200_flags_from_driver_data(flags); mdev->type = mgag200_type_from_driver_data(flags); /* Hardcode the number of CRTCs to 1 */ -- 2.23.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-11-26 7:25 [PATCH 0/4] drm/mgag200: Workaround HW bug with non-0 offset Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 1/4] drm/mgag200: Extract device type from flags Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 2/4] drm/mgag200: Store flags from PCI driver data in device structure Thomas Zimmermann @ 2019-11-26 7:25 ` Thomas Zimmermann 2019-11-26 9:37 ` Daniel Vetter 2019-11-26 7:25 ` [PATCH 4/4] drm/mgag200: Add module parameter to pin all buffers at offset 0 Thomas Zimmermann 3 siblings, 1 reply; 14+ messages in thread From: Thomas Zimmermann @ 2019-11-26 7:25 UTC (permalink / raw) To: airlied, daniel, john.p.donnelly, kraxel, sam Cc: Thomas Zimmermann, dri-devel There's at least one system that does not interpret the value of the device's 'startadd' field correctly, which leads to incorrectly displayed scanout buffers. Always placing the active scanout buffer at offset 0 works around the problem. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Reported-by: John Donnelly <john.p.donnelly@oracle.com> Link: https://gitlab.freedesktop.org/drm/misc/issues/7 --- drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 397f8b0a9af8..d43951caeea0 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); static struct drm_driver driver; static const struct pci_device_id pciidlist[] = { + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, { PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_A }, { PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B }, { PCI_VENDOR_ID_MATROX, 0x530, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_EV }, @@ -60,6 +62,35 @@ 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) +{ + 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, &dev->vram_mm->bdev, + pg_align, false, args); +} + static struct drm_driver driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET, .load = mgag200_driver_load, @@ -71,7 +102,10 @@ static struct drm_driver driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, - DRM_GEM_VRAM_DRIVER + .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, }; 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 4b4f9ce74a84..aa32aad222c2 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -150,6 +150,9 @@ enum mga_type { G200_EW3, }; +/* HW does not handle 'startadd' field correct. */ +#define MGAG200_FLAG_HW_BUG_NO_STARTADD (1ul << 8) + #define MGAG200_TYPE_MASK (0x000000ff) #define MGAG200_FLAG_MASK (0x00ffff00) -- 2.23.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-11-26 7:25 ` [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' Thomas Zimmermann @ 2019-11-26 9:37 ` Daniel Vetter 2019-11-26 9:50 ` Thomas Zimmermann 0 siblings, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2019-11-26 9:37 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: john.p.donnelly, dri-devel, kraxel, airlied, sam On Tue, Nov 26, 2019 at 08:25:44AM +0100, Thomas Zimmermann wrote: > There's at least one system that does not interpret the value of > the device's 'startadd' field correctly, which leads to incorrectly > displayed scanout buffers. Always placing the active scanout buffer > at offset 0 works around the problem. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Reported-by: John Donnelly <john.p.donnelly@oracle.com> > Link: https://gitlab.freedesktop.org/drm/misc/issues/7 Tested-by: John Donnelly <john.p.donnelly@oracle.com> (Not quite this patch, but pretty much the logic, so counts). Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin") Cc: <stable@vger.kernel.org> # v5.3+ Also you need the stable line on both prep patches too. For next time around, $ dim fixes 81da87f63a1e will generate all the stuff you need, including a good set of suggested Cc: you should have. On the first 3 patches, with all that stuff added: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Please push these to drm-misc-next-fixes so they get backported as quickly as possible. -Daniel > --- > drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- > drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ > 2 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c > index 397f8b0a9af8..d43951caeea0 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c > @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); > static struct drm_driver driver; > > static const struct pci_device_id pciidlist[] = { > + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, > + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, > { PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_A }, > { PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B }, > { PCI_VENDOR_ID_MATROX, 0x530, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_EV }, > @@ -60,6 +62,35 @@ 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) > +{ > + 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, &dev->vram_mm->bdev, > + pg_align, false, args); > +} > + > static struct drm_driver driver = { > .driver_features = DRIVER_GEM | DRIVER_MODESET, > .load = mgag200_driver_load, > @@ -71,7 +102,10 @@ static struct drm_driver driver = { > .major = DRIVER_MAJOR, > .minor = DRIVER_MINOR, > .patchlevel = DRIVER_PATCHLEVEL, > - DRM_GEM_VRAM_DRIVER > + .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, > }; > > 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 4b4f9ce74a84..aa32aad222c2 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h > @@ -150,6 +150,9 @@ enum mga_type { > G200_EW3, > }; > > +/* HW does not handle 'startadd' field correct. */ > +#define MGAG200_FLAG_HW_BUG_NO_STARTADD (1ul << 8) > + > #define MGAG200_TYPE_MASK (0x000000ff) > #define MGAG200_FLAG_MASK (0x00ffff00) > > -- > 2.23.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-11-26 9:37 ` Daniel Vetter @ 2019-11-26 9:50 ` Thomas Zimmermann 2019-12-03 17:55 ` John Donnelly 0 siblings, 1 reply; 14+ messages in thread From: Thomas Zimmermann @ 2019-11-26 9:50 UTC (permalink / raw) To: Daniel Vetter; +Cc: airlied, sam, kraxel, dri-devel, john.p.donnelly [-- Attachment #1.1.1: Type: text/plain, Size: 4854 bytes --] Hi Am 26.11.19 um 10:37 schrieb Daniel Vetter: > On Tue, Nov 26, 2019 at 08:25:44AM +0100, Thomas Zimmermann wrote: >> There's at least one system that does not interpret the value of >> the device's 'startadd' field correctly, which leads to incorrectly >> displayed scanout buffers. Always placing the active scanout buffer >> at offset 0 works around the problem. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Reported-by: John Donnelly <john.p.donnelly@oracle.com> >> Link: https://gitlab.freedesktop.org/drm/misc/issues/7 > > Tested-by: John Donnelly <john.p.donnelly@oracle.com> > > (Not quite this patch, but pretty much the logic, so counts). > > Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin") > Cc: <stable@vger.kernel.org> # v5.3+ > > Also you need the stable line on both prep patches too. For next time > around, > > $ dim fixes 81da87f63a1e > > will generate all the stuff you need, including a good set of suggested > Cc: you should have. > > On the first 3 patches, with all that stuff added: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks for the review. Sorry for leaving out some of the tags. I wanted to wait for feedback before adding tested-by, fixes, stable. I'll split off patch 4 from the series and get 1 to 3 merged ASAP. Best regards Thomas > > Please push these to drm-misc-next-fixes so they get backported as quickly > as possible. > -Daniel > >> --- >> drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- >> drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ >> 2 files changed, 38 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c >> index 397f8b0a9af8..d43951caeea0 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c >> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c >> @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); >> static struct drm_driver driver; >> >> static const struct pci_device_id pciidlist[] = { >> + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, >> + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, >> { PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_A }, >> { PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B }, >> { PCI_VENDOR_ID_MATROX, 0x530, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_EV }, >> @@ -60,6 +62,35 @@ 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) >> +{ >> + 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, &dev->vram_mm->bdev, >> + pg_align, false, args); >> +} >> + >> static struct drm_driver driver = { >> .driver_features = DRIVER_GEM | DRIVER_MODESET, >> .load = mgag200_driver_load, >> @@ -71,7 +102,10 @@ static struct drm_driver driver = { >> .major = DRIVER_MAJOR, >> .minor = DRIVER_MINOR, >> .patchlevel = DRIVER_PATCHLEVEL, >> - DRM_GEM_VRAM_DRIVER >> + .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, >> }; >> >> 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 4b4f9ce74a84..aa32aad222c2 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h >> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h >> @@ -150,6 +150,9 @@ enum mga_type { >> G200_EW3, >> }; >> >> +/* HW does not handle 'startadd' field correct. */ >> +#define MGAG200_FLAG_HW_BUG_NO_STARTADD (1ul << 8) >> + >> #define MGAG200_TYPE_MASK (0x000000ff) >> #define MGAG200_FLAG_MASK (0x00ffff00) >> >> -- >> 2.23.0 >> > -- 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 [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-11-26 9:50 ` Thomas Zimmermann @ 2019-12-03 17:55 ` John Donnelly 2019-12-04 7:30 ` Thomas Zimmermann 0 siblings, 1 reply; 14+ messages in thread From: John Donnelly @ 2019-12-03 17:55 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: airlied, dri-devel, sam, kraxel Hi , See below , > On Nov 26, 2019, at 3:50 AM, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 26.11.19 um 10:37 schrieb Daniel Vetter: >> On Tue, Nov 26, 2019 at 08:25:44AM +0100, Thomas Zimmermann wrote: >>> There's at least one system that does not interpret the value of >>> the device's 'startadd' field correctly, which leads to incorrectly >>> displayed scanout buffers. Always placing the active scanout buffer >>> at offset 0 works around the problem. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Reported-by: John Donnelly <john.p.donnelly@oracle.com> >>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7 >> >> Tested-by: John Donnelly <john.p.donnelly@oracle.com> >> >> (Not quite this patch, but pretty much the logic, so counts). >> >> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin") >> Cc: <stable@vger.kernel.org> # v5.3+ >> >> Also you need the stable line on both prep patches too. For next time >> around, >> >> $ dim fixes 81da87f63a1e >> >> will generate all the stuff you need, including a good set of suggested >> Cc: you should have. >> >> On the first 3 patches, with all that stuff added: >> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Thanks for the review. > > Sorry for leaving out some of the tags. I wanted to wait for feedback > before adding tested-by, fixes, stable. I'll split off patch 4 from the > series and get 1 to 3 merged ASAP. > > Best regards > Thomas > >> >> Please push these to drm-misc-next-fixes so they get backported as quickly >> as possible. >> -Daniel >> >>> --- >>> drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- >>> drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ >>> 2 files changed, 38 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c >>> index 397f8b0a9af8..d43951caeea0 100644 >>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c >>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c >>> @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); >>> static struct drm_driver driver; >>> >>> static const struct pci_device_id pciidlist[] = { >>> + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, >>> + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, I will have an additional list of vendor IDs (0x4852) I will provide in a follow up patch shortly . This fixes only 1 flavor of server. Thank you . >>> { PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_A }, >>> { PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B }, >>> { PCI_VENDOR_ID_MATROX, 0x530, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_EV }, >>> @@ -60,6 +62,35 @@ 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) >>> +{ >>> + 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, &dev->vram_mm->bdev, >>> + pg_align, false, args); >>> +} >>> + >>> static struct drm_driver driver = { >>> .driver_features = DRIVER_GEM | DRIVER_MODESET, >>> .load = mgag200_driver_load, >>> @@ -71,7 +102,10 @@ static struct drm_driver driver = { >>> .major = DRIVER_MAJOR, >>> .minor = DRIVER_MINOR, >>> .patchlevel = DRIVER_PATCHLEVEL, >>> - DRM_GEM_VRAM_DRIVER >>> + .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, >>> }; >>> >>> 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 4b4f9ce74a84..aa32aad222c2 100644 >>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h >>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h >>> @@ -150,6 +150,9 @@ enum mga_type { >>> G200_EW3, >>> }; >>> >>> +/* HW does not handle 'startadd' field correct. */ >>> +#define MGAG200_FLAG_HW_BUG_NO_STARTADD (1ul << 8) >>> + >>> #define MGAG200_TYPE_MASK (0x000000ff) >>> #define MGAG200_FLAG_MASK (0x00ffff00) >>> >>> -- >>> 2.23.0 >>> >> > > -- > 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 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-12-03 17:55 ` John Donnelly @ 2019-12-04 7:30 ` Thomas Zimmermann 2019-12-04 9:36 ` Dave Airlie 0 siblings, 1 reply; 14+ messages in thread From: Thomas Zimmermann @ 2019-12-04 7:30 UTC (permalink / raw) To: John Donnelly; +Cc: airlied, dri-devel, sam, kraxel [-- Attachment #1.1.1: Type: text/plain, Size: 6307 bytes --] Hi John Am 03.12.19 um 18:55 schrieb John Donnelly: > Hi , > > See below , > > >> On Nov 26, 2019, at 3:50 AM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 26.11.19 um 10:37 schrieb Daniel Vetter: >>> On Tue, Nov 26, 2019 at 08:25:44AM +0100, Thomas Zimmermann wrote: >>>> There's at least one system that does not interpret the value of >>>> the device's 'startadd' field correctly, which leads to incorrectly >>>> displayed scanout buffers. Always placing the active scanout buffer >>>> at offset 0 works around the problem. >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> Reported-by: John Donnelly <john.p.donnelly@oracle.com> >>>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7 >>> >>> Tested-by: John Donnelly <john.p.donnelly@oracle.com> >>> >>> (Not quite this patch, but pretty much the logic, so counts). >>> >>> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin") >>> Cc: <stable@vger.kernel.org> # v5.3+ >>> >>> Also you need the stable line on both prep patches too. For next time >>> around, >>> >>> $ dim fixes 81da87f63a1e >>> >>> will generate all the stuff you need, including a good set of suggested >>> Cc: you should have. >>> >>> On the first 3 patches, with all that stuff added: >>> >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> Thanks for the review. >> >> Sorry for leaving out some of the tags. I wanted to wait for feedback >> before adding tested-by, fixes, stable. I'll split off patch 4 from the >> series and get 1 to 3 merged ASAP. >> >> Best regards >> Thomas >> >>> >>> Please push these to drm-misc-next-fixes so they get backported as quickly >>> as possible. >>> -Daniel >>> >>>> --- >>>> drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- >>>> drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ >>>> 2 files changed, 38 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c >>>> index 397f8b0a9af8..d43951caeea0 100644 >>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c >>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c >>>> @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); >>>> static struct drm_driver driver; >>>> >>>> static const struct pci_device_id pciidlist[] = { >>>> + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, >>>> + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, > > > > I will have an additional list of vendor IDs (0x4852) I will provide in a follow up patch shortly . This fixes only 1 flavor of server. Thank you for all the testing you do. We can add these ids as necessary. Before, I posted a patch at [1] that prints an internal unique id. The id of your original test machine was 0x2 IIRC. My guess is that the problem might be related to the chip's revision. If you have the option of booting your own kernels on all these machines, could you apply the patch and look for a pattern in these ids? Maybe only revision 0x2 is affected. Best regards Thomas [1] https://gitlab.freedesktop.org/drm/misc/uploads/1d5d9a75571c2bac71f80e0d2411e840/0001-drm-mgag200-Print-unique-revision-id-for-G200SE.patch > > > Thank you . > > > > > > >>>> { PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_A }, >>>> { PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B }, >>>> { PCI_VENDOR_ID_MATROX, 0x530, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_EV }, >>>> @@ -60,6 +62,35 @@ 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) >>>> +{ >>>> + 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, &dev->vram_mm->bdev, >>>> + pg_align, false, args); >>>> +} >>>> + >>>> static struct drm_driver driver = { >>>> .driver_features = DRIVER_GEM | DRIVER_MODESET, >>>> .load = mgag200_driver_load, >>>> @@ -71,7 +102,10 @@ static struct drm_driver driver = { >>>> .major = DRIVER_MAJOR, >>>> .minor = DRIVER_MINOR, >>>> .patchlevel = DRIVER_PATCHLEVEL, >>>> - DRM_GEM_VRAM_DRIVER >>>> + .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, >>>> }; >>>> >>>> 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 4b4f9ce74a84..aa32aad222c2 100644 >>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h >>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h >>>> @@ -150,6 +150,9 @@ enum mga_type { >>>> G200_EW3, >>>> }; >>>> >>>> +/* HW does not handle 'startadd' field correct. */ >>>> +#define MGAG200_FLAG_HW_BUG_NO_STARTADD (1ul << 8) >>>> + >>>> #define MGAG200_TYPE_MASK (0x000000ff) >>>> #define MGAG200_FLAG_MASK (0x00ffff00) >>>> >>>> -- >>>> 2.23.0 >>>> >>> >> >> -- >> 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 >> > -- 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 [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-12-04 7:30 ` Thomas Zimmermann @ 2019-12-04 9:36 ` Dave Airlie 2019-12-06 6:14 ` Thomas Zimmermann 0 siblings, 1 reply; 14+ messages in thread From: Dave Airlie @ 2019-12-04 9:36 UTC (permalink / raw) To: Thomas Zimmermann Cc: John Donnelly, Sam Ravnborg, Gerd Hoffmann, dri-devel, Dave Airlie On Wed, 4 Dec 2019 at 17:30, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi John > > Am 03.12.19 um 18:55 schrieb John Donnelly: > > Hi , > > > > See below , > > > > > >> On Nov 26, 2019, at 3:50 AM, Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> > >> Hi > >> > >> Am 26.11.19 um 10:37 schrieb Daniel Vetter: > >>> On Tue, Nov 26, 2019 at 08:25:44AM +0100, Thomas Zimmermann wrote: > >>>> There's at least one system that does not interpret the value of > >>>> the device's 'startadd' field correctly, which leads to incorrectly > >>>> displayed scanout buffers. Always placing the active scanout buffer > >>>> at offset 0 works around the problem. > >>>> > >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >>>> Reported-by: John Donnelly <john.p.donnelly@oracle.com> > >>>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7 > >>> > >>> Tested-by: John Donnelly <john.p.donnelly@oracle.com> > >>> > >>> (Not quite this patch, but pretty much the logic, so counts). > >>> > >>> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin") > >>> Cc: <stable@vger.kernel.org> # v5.3+ > >>> > >>> Also you need the stable line on both prep patches too. For next time > >>> around, > >>> > >>> $ dim fixes 81da87f63a1e > >>> > >>> will generate all the stuff you need, including a good set of suggested > >>> Cc: you should have. > >>> > >>> On the first 3 patches, with all that stuff added: > >>> > >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > >> Thanks for the review. > >> > >> Sorry for leaving out some of the tags. I wanted to wait for feedback > >> before adding tested-by, fixes, stable. I'll split off patch 4 from the > >> series and get 1 to 3 merged ASAP. > >> > >> Best regards > >> Thomas > >> > >>> > >>> Please push these to drm-misc-next-fixes so they get backported as quickly > >>> as possible. > >>> -Daniel > >>> > >>>> --- > >>>> drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- > >>>> drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ > >>>> 2 files changed, 38 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c > >>>> index 397f8b0a9af8..d43951caeea0 100644 > >>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c > >>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c > >>>> @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); > >>>> static struct drm_driver driver; > >>>> > >>>> static const struct pci_device_id pciidlist[] = { > >>>> + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, > >>>> + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, > > > > > > > > I will have an additional list of vendor IDs (0x4852) I will provide in a follow up patch shortly . This fixes only 1 flavor of server. > > Thank you for all the testing you do. We can add these ids as necessary. > > Before, I posted a patch at [1] that prints an internal unique id. The > id of your original test machine was 0x2 IIRC. > > My guess is that the problem might be related to the chip's revision. If > you have the option of booting your own kernels on all these machines, > could you apply the patch and look for a pattern in these ids? Maybe > only revision 0x2 is affected. > I've got an old bug I never got around to that has a comment from Matrox "The issue is reproducible with G200e chip. The device ID is 0x0522." so it might be a broader problem than we think. Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-12-04 9:36 ` Dave Airlie @ 2019-12-06 6:14 ` Thomas Zimmermann 2019-12-06 6:50 ` David Airlie 0 siblings, 1 reply; 14+ messages in thread From: Thomas Zimmermann @ 2019-12-06 6:14 UTC (permalink / raw) To: Dave Airlie Cc: John Donnelly, Sam Ravnborg, Gerd Hoffmann, dri-devel, Dave Airlie [-- Attachment #1.1.1: Type: text/plain, Size: 4270 bytes --] Hi Am 04.12.19 um 10:36 schrieb Dave Airlie: > On Wed, 4 Dec 2019 at 17:30, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi John >> >> Am 03.12.19 um 18:55 schrieb John Donnelly: >>> Hi , >>> >>> See below , >>> >>> >>>> On Nov 26, 2019, at 3:50 AM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> >>>> Hi >>>> >>>> Am 26.11.19 um 10:37 schrieb Daniel Vetter: >>>>> On Tue, Nov 26, 2019 at 08:25:44AM +0100, Thomas Zimmermann wrote: >>>>>> There's at least one system that does not interpret the value of >>>>>> the device's 'startadd' field correctly, which leads to incorrectly >>>>>> displayed scanout buffers. Always placing the active scanout buffer >>>>>> at offset 0 works around the problem. >>>>>> >>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>> Reported-by: John Donnelly <john.p.donnelly@oracle.com> >>>>>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7 >>>>> >>>>> Tested-by: John Donnelly <john.p.donnelly@oracle.com> >>>>> >>>>> (Not quite this patch, but pretty much the logic, so counts). >>>>> >>>>> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin") >>>>> Cc: <stable@vger.kernel.org> # v5.3+ >>>>> >>>>> Also you need the stable line on both prep patches too. For next time >>>>> around, >>>>> >>>>> $ dim fixes 81da87f63a1e >>>>> >>>>> will generate all the stuff you need, including a good set of suggested >>>>> Cc: you should have. >>>>> >>>>> On the first 3 patches, with all that stuff added: >>>>> >>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> >>>> Thanks for the review. >>>> >>>> Sorry for leaving out some of the tags. I wanted to wait for feedback >>>> before adding tested-by, fixes, stable. I'll split off patch 4 from the >>>> series and get 1 to 3 merged ASAP. >>>> >>>> Best regards >>>> Thomas >>>> >>>>> >>>>> Please push these to drm-misc-next-fixes so they get backported as quickly >>>>> as possible. >>>>> -Daniel >>>>> >>>>>> --- >>>>>> drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- >>>>>> drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ >>>>>> 2 files changed, 38 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c >>>>>> index 397f8b0a9af8..d43951caeea0 100644 >>>>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c >>>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c >>>>>> @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); >>>>>> static struct drm_driver driver; >>>>>> >>>>>> static const struct pci_device_id pciidlist[] = { >>>>>> + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, >>>>>> + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, >>> >>> >>> >>> I will have an additional list of vendor IDs (0x4852) I will provide in a follow up patch shortly . This fixes only 1 flavor of server. >> >> Thank you for all the testing you do. We can add these ids as necessary. >> >> Before, I posted a patch at [1] that prints an internal unique id. The >> id of your original test machine was 0x2 IIRC. >> >> My guess is that the problem might be related to the chip's revision. If >> you have the option of booting your own kernels on all these machines, >> could you apply the patch and look for a pattern in these ids? Maybe >> only revision 0x2 is affected. >> > > I've got an old bug I never got around to that has a comment from Matrox > > "The issue is reproducible with G200e chip. The device ID is 0x0522." > > so it might be a broader problem than we think. Did they tell you a subvendor id? John reported that the internal revision id differs among affected machines. I'm thinking about flagging either Sun devices or all 0x0522 devices as broken. Best regards Thomas > > Dave. > _______________________________________________ > 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 [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-12-06 6:14 ` Thomas Zimmermann @ 2019-12-06 6:50 ` David Airlie 2019-12-06 7:51 ` Thomas Zimmermann 0 siblings, 1 reply; 14+ messages in thread From: David Airlie @ 2019-12-06 6:50 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: John Donnelly, dri-devel, Gerd Hoffmann, Sam Ravnborg On Fri, Dec 6, 2019 at 4:14 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 04.12.19 um 10:36 schrieb Dave Airlie: > > On Wed, 4 Dec 2019 at 17:30, Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> > >> Hi John > >> > >> Am 03.12.19 um 18:55 schrieb John Donnelly: > >>> Hi , > >>> > >>> See below , > >>> > >>> > >>>> On Nov 26, 2019, at 3:50 AM, Thomas Zimmermann <tzimmermann@suse.de> wrote: > >>>> > >>>> Hi > >>>> > >>>> Am 26.11.19 um 10:37 schrieb Daniel Vetter: > >>>>> On Tue, Nov 26, 2019 at 08:25:44AM +0100, Thomas Zimmermann wrote: > >>>>>> There's at least one system that does not interpret the value of > >>>>>> the device's 'startadd' field correctly, which leads to incorrectly > >>>>>> displayed scanout buffers. Always placing the active scanout buffer > >>>>>> at offset 0 works around the problem. > >>>>>> > >>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >>>>>> Reported-by: John Donnelly <john.p.donnelly@oracle.com> > >>>>>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7 > >>>>> > >>>>> Tested-by: John Donnelly <john.p.donnelly@oracle.com> > >>>>> > >>>>> (Not quite this patch, but pretty much the logic, so counts). > >>>>> > >>>>> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin") > >>>>> Cc: <stable@vger.kernel.org> # v5.3+ > >>>>> > >>>>> Also you need the stable line on both prep patches too. For next time > >>>>> around, > >>>>> > >>>>> $ dim fixes 81da87f63a1e > >>>>> > >>>>> will generate all the stuff you need, including a good set of suggested > >>>>> Cc: you should have. > >>>>> > >>>>> On the first 3 patches, with all that stuff added: > >>>>> > >>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>> > >>>> Thanks for the review. > >>>> > >>>> Sorry for leaving out some of the tags. I wanted to wait for feedback > >>>> before adding tested-by, fixes, stable. I'll split off patch 4 from the > >>>> series and get 1 to 3 merged ASAP. > >>>> > >>>> Best regards > >>>> Thomas > >>>> > >>>>> > >>>>> Please push these to drm-misc-next-fixes so they get backported as quickly > >>>>> as possible. > >>>>> -Daniel > >>>>> > >>>>>> --- > >>>>>> drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- > >>>>>> drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ > >>>>>> 2 files changed, 38 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c > >>>>>> index 397f8b0a9af8..d43951caeea0 100644 > >>>>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c > >>>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c > >>>>>> @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); > >>>>>> static struct drm_driver driver; > >>>>>> > >>>>>> static const struct pci_device_id pciidlist[] = { > >>>>>> + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, > >>>>>> + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, > >>> > >>> > >>> > >>> I will have an additional list of vendor IDs (0x4852) I will provide in a follow up patch shortly . This fixes only 1 flavor of server. > >> > >> Thank you for all the testing you do. We can add these ids as necessary. > >> > >> Before, I posted a patch at [1] that prints an internal unique id. The > >> id of your original test machine was 0x2 IIRC. > >> > >> My guess is that the problem might be related to the chip's revision. If > >> you have the option of booting your own kernels on all these machines, > >> could you apply the patch and look for a pattern in these ids? Maybe > >> only revision 0x2 is affected. > >> > > > > I've got an old bug I never got around to that has a comment from Matrox > > > > "The issue is reproducible with G200e chip. The device ID is 0x0522." > > > > so it might be a broader problem than we think. > > Did they tell you a subvendor id? John reported that the internal > revision id differs among affected machines. I'm thinking about flagging > either Sun devices or all 0x0522 devices as broken. Well it was from Matrox themselves, so they are the vendor ID, it didn't sounds like subvendor mattered, though I expect the problem is the BMC firmware anyways, not sure if we can even know what ths is. Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' 2019-12-06 6:50 ` David Airlie @ 2019-12-06 7:51 ` Thomas Zimmermann 0 siblings, 0 replies; 14+ messages in thread From: Thomas Zimmermann @ 2019-12-06 7:51 UTC (permalink / raw) To: David Airlie; +Cc: John Donnelly, Sam Ravnborg, Gerd Hoffmann, dri-devel [-- Attachment #1.1.1: Type: text/plain, Size: 4895 bytes --] Hi Am 06.12.19 um 07:50 schrieb David Airlie: > On Fri, Dec 6, 2019 at 4:14 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 04.12.19 um 10:36 schrieb Dave Airlie: >>> On Wed, 4 Dec 2019 at 17:30, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> >>>> Hi John >>>> >>>> Am 03.12.19 um 18:55 schrieb John Donnelly: >>>>> Hi , >>>>> >>>>> See below , >>>>> >>>>> >>>>>> On Nov 26, 2019, at 3:50 AM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>>>> >>>>>> Hi >>>>>> >>>>>> Am 26.11.19 um 10:37 schrieb Daniel Vetter: >>>>>>> On Tue, Nov 26, 2019 at 08:25:44AM +0100, Thomas Zimmermann wrote: >>>>>>>> There's at least one system that does not interpret the value of >>>>>>>> the device's 'startadd' field correctly, which leads to incorrectly >>>>>>>> displayed scanout buffers. Always placing the active scanout buffer >>>>>>>> at offset 0 works around the problem. >>>>>>>> >>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>>> Reported-by: John Donnelly <john.p.donnelly@oracle.com> >>>>>>>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7 >>>>>>> >>>>>>> Tested-by: John Donnelly <john.p.donnelly@oracle.com> >>>>>>> >>>>>>> (Not quite this patch, but pretty much the logic, so counts). >>>>>>> >>>>>>> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin") >>>>>>> Cc: <stable@vger.kernel.org> # v5.3+ >>>>>>> >>>>>>> Also you need the stable line on both prep patches too. For next time >>>>>>> around, >>>>>>> >>>>>>> $ dim fixes 81da87f63a1e >>>>>>> >>>>>>> will generate all the stuff you need, including a good set of suggested >>>>>>> Cc: you should have. >>>>>>> >>>>>>> On the first 3 patches, with all that stuff added: >>>>>>> >>>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>> >>>>>> Thanks for the review. >>>>>> >>>>>> Sorry for leaving out some of the tags. I wanted to wait for feedback >>>>>> before adding tested-by, fixes, stable. I'll split off patch 4 from the >>>>>> series and get 1 to 3 merged ASAP. >>>>>> >>>>>> Best regards >>>>>> Thomas >>>>>> >>>>>>> >>>>>>> Please push these to drm-misc-next-fixes so they get backported as quickly >>>>>>> as possible. >>>>>>> -Daniel >>>>>>> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/mgag200/mgag200_drv.c | 36 ++++++++++++++++++++++++++- >>>>>>>> drivers/gpu/drm/mgag200/mgag200_drv.h | 3 +++ >>>>>>>> 2 files changed, 38 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c >>>>>>>> index 397f8b0a9af8..d43951caeea0 100644 >>>>>>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c >>>>>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c >>>>>>>> @@ -30,6 +30,8 @@ module_param_named(modeset, mgag200_modeset, int, 0400); >>>>>>>> static struct drm_driver driver; >>>>>>>> >>>>>>>> static const struct pci_device_id pciidlist[] = { >>>>>>>> + { PCI_VENDOR_ID_MATROX, 0x522, PCI_VENDOR_ID_SUN, 0x4852, 0, 0, >>>>>>>> + G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD}, >>>>> >>>>> >>>>> >>>>> I will have an additional list of vendor IDs (0x4852) I will provide in a follow up patch shortly . This fixes only 1 flavor of server. >>>> >>>> Thank you for all the testing you do. We can add these ids as necessary. >>>> >>>> Before, I posted a patch at [1] that prints an internal unique id. The >>>> id of your original test machine was 0x2 IIRC. >>>> >>>> My guess is that the problem might be related to the chip's revision. If >>>> you have the option of booting your own kernels on all these machines, >>>> could you apply the patch and look for a pattern in these ids? Maybe >>>> only revision 0x2 is affected. >>>> >>> >>> I've got an old bug I never got around to that has a comment from Matrox >>> >>> "The issue is reproducible with G200e chip. The device ID is 0x0522." >>> >>> so it might be a broader problem than we think. >> >> Did they tell you a subvendor id? John reported that the internal >> revision id differs among affected machines. I'm thinking about flagging >> either Sun devices or all 0x0522 devices as broken. > > Well it was from Matrox themselves, so they are the vendor ID, it > didn't sounds like subvendor mattered, though I expect the problem is > the BMC firmware anyways, not sure if we can even know what ths is. OK, thanks. I'll prepare a patch to flag all 0x0522 machines. Best regards Thomas > > Dave. > > _______________________________________________ > 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 [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] drm/mgag200: Add module parameter to pin all buffers at offset 0 2019-11-26 7:25 [PATCH 0/4] drm/mgag200: Workaround HW bug with non-0 offset Thomas Zimmermann ` (2 preceding siblings ...) 2019-11-26 7:25 ` [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' Thomas Zimmermann @ 2019-11-26 7:25 ` Thomas Zimmermann 2019-11-26 9:38 ` Daniel Vetter 3 siblings, 1 reply; 14+ messages in thread From: Thomas Zimmermann @ 2019-11-26 7:25 UTC (permalink / raw) To: airlied, daniel, john.p.donnelly, kraxel, sam Cc: Thomas Zimmermann, dri-devel For hardware that does not interpret the startadd field correctly, add the module parameter 'hw_bug_no_startadd', which enables the workaround. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_drv.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index d43951caeea0..79836b09a54a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -27,6 +27,10 @@ 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[] = { @@ -64,6 +68,10 @@ 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) + return false; + else if (mgag200_hw_bug_no_startadd > 0) + return true; return mdev->flags & MGAG200_FLAG_HW_BUG_NO_STARTADD; } -- 2.23.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] drm/mgag200: Add module parameter to pin all buffers at offset 0 2019-11-26 7:25 ` [PATCH 4/4] drm/mgag200: Add module parameter to pin all buffers at offset 0 Thomas Zimmermann @ 2019-11-26 9:38 ` Daniel Vetter 0 siblings, 0 replies; 14+ messages in thread From: Daniel Vetter @ 2019-11-26 9:38 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: john.p.donnelly, dri-devel, kraxel, airlied, sam On Tue, Nov 26, 2019 at 08:25:45AM +0100, Thomas Zimmermann wrote: > For hardware that does not interpret the startadd field correctly, > add the module parameter 'hw_bug_no_startadd', which enables the > workaround. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/mgag200/mgag200_drv.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c > index d43951caeea0..79836b09a54a 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c > @@ -27,6 +27,10 @@ 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[] = { > @@ -64,6 +68,10 @@ DEFINE_DRM_GEM_FOPS(mgag200_driver_fops); > > static bool mgag200_pin_bo_at_0(const struct mga_device *mdev) > { I'd add a infor printing if the module option is non-zero that people should submit a bug report if this fixes their issue. We don't want people fixing bugs with module options, stuff should work by default. With that addressed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + if (!mgag200_hw_bug_no_startadd) > + return false; > + else if (mgag200_hw_bug_no_startadd > 0) > + return true; > return mdev->flags & MGAG200_FLAG_HW_BUG_NO_STARTADD; > } > > -- > 2.23.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-12-06 7:51 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-26 7:25 [PATCH 0/4] drm/mgag200: Workaround HW bug with non-0 offset Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 1/4] drm/mgag200: Extract device type from flags Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 2/4] drm/mgag200: Store flags from PCI driver data in device structure Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd' Thomas Zimmermann 2019-11-26 9:37 ` Daniel Vetter 2019-11-26 9:50 ` Thomas Zimmermann 2019-12-03 17:55 ` John Donnelly 2019-12-04 7:30 ` Thomas Zimmermann 2019-12-04 9:36 ` Dave Airlie 2019-12-06 6:14 ` Thomas Zimmermann 2019-12-06 6:50 ` David Airlie 2019-12-06 7:51 ` Thomas Zimmermann 2019-11-26 7:25 ` [PATCH 4/4] drm/mgag200: Add module parameter to pin all buffers at offset 0 Thomas Zimmermann 2019-11-26 9:38 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.