Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
Hi
Am 06.12.19 um 07:50 schrieb David Airlie:
> On Fri, Dec 6, 2019 at 4:14 PM Thomas Zimmermann wrote:
>>
>> Hi
>>
>> Am 04.12.19 um 10:36 schrieb Dave Airlie:
>>> On Wed, 4 Dec 2019 at 17:30, Thomas Zimmermann 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
>> 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
Reported-by: John Donnelly
Link: https://gitlab.freedesktop.org/drm/misc/issues/7
>>>
>>> Tested-by: John Donnelly
>>>
>>> (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: # 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
>>
>> 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
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 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
On Fri, Dec 6, 2019 at 4:14 PM Thomas Zimmermann wrote:
>
> Hi
>
> Am 04.12.19 um 10:36 schrieb Dave Airlie:
> > On Wed, 4 Dec 2019 at 17:30, Thomas Zimmermann 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
> 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
> >> Reported-by: John Donnelly
> >> Link: https://gitlab.freedesktop.org/drm/misc/issues/7
> >
> > Tested-by: John Donnelly
> >
> > (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: # 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
>
> 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
Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
Hi
Am 04.12.19 um 10:36 schrieb Dave Airlie:
> On Wed, 4 Dec 2019 at 17:30, Thomas Zimmermann 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 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
>> Reported-by: John Donnelly
>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7
>
> Tested-by: John Donnelly
>
> (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: # 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
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
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 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
On Wed, 4 Dec 2019 at 17:30, Thomas Zimmermann 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 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
> Reported-by: John Donnelly
> Link: https://gitlab.freedesktop.org/drm/misc/issues/7
> >>>
> >>> Tested-by: John Donnelly
> >>>
> >>> (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: # 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
> >>
> >> 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
Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
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 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
Reported-by: John Donnelly
Link: https://gitlab.freedesktop.org/drm/misc/issues/7
>>>
>>> Tested-by: John Donnelly
>>>
>>> (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: # 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
>>
>> 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, >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 =
Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
Hi ,
See below ,
> On Nov 26, 2019, at 3:50 AM, Thomas Zimmermann 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
>>> Reported-by: John Donnelly
>>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7
>>
>> Tested-by: John Donnelly
>>
>> (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: # 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
>
> 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, >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 (0x00ff)
>>> #define MGAG200_FLAG_MASK (0x0000)
>>>
>>> --
>>> 2.23.0
>>>
>>
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
>
Re: [PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
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
>> Reported-by: John Donnelly
>> Link: https://gitlab.freedesktop.org/drm/misc/issues/7
>
> Tested-by: John Donnelly
>
> (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: # 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
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, >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 (0x00ff)
>> #define MGAG200_FLAG_MASK (0x0000)
>>
>> --
>> 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
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 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
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
> Reported-by: John Donnelly
> Link: https://gitlab.freedesktop.org/drm/misc/issues/7
Tested-by: John Donnelly
(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: # 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
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, >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(0x00ff)
> #define MGAG200_FLAG_MASK(0x0000)
>
> --
> 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
[PATCH 3/4] drm/mgag200: Add workaround for HW that does not support 'startadd'
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
Reported-by: John Donnelly
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, >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 (0x00ff)
#define MGAG200_FLAG_MASK (0x0000)
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
