[PATCH v2 1/3] drm/mgag200: Extract device type from flags

[PATCH v2 1/3] drm/mgag200: Extract device type from flags

[Thomas Zimmermann]

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>
Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin")
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: John Donnelly <john.p.donnelly@oracle.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "José Roberto de Souza" <jose.souza@intel.com>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.3+
---
 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

Re: [PATCH v2 1/3] drm/mgag200: Extract device type from flags

[Emil Velikov]

Hi Thomas,

On Tue, 26 Nov 2019 at 10:15, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> 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>
> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin")

Are you sure the fixes tag is correct? Neither the commit summary nor
the patch itself seems related to the changes below.

HTH
Emil

Re: [PATCH v2 1/3] drm/mgag200: Extract device type from flags

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1408 bytes --]

Hi Emil

Am 27.11.19 um 17:29 schrieb Emil Velikov:
> Hi Thomas,
> 
> On Tue, 26 Nov 2019 at 10:15, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> 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>
>> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin")
> 
> Are you sure the fixes tag is correct? Neither the commit summary nor
> the patch itself seems related to the changes below.

Yes, it's correct. It's part of a patch series [1][2][3] that fixes the bug.

Best regards
Thomas

[1]
https://cgit.freedesktop.org/drm/drm-tip/commit/?id=3a8a5aba142a44eaeba0cb0ec1b4a8f177b5e59a
[2]
https://cgit.freedesktop.org/drm/drm-tip/commit/?id=d6d437d97d54c85a1a93967b2745e31dff03365a
[3]
https://cgit.freedesktop.org/drm/drm-tip/commit/?id=1591fadf857cdbaf2baa55e421af99a61354713c

> 
> HTH
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] drm/mgag200: Extract device type from flags

[Emil Velikov]

On 2019/11/27, Thomas Zimmermann wrote:
> Hi Emil
> 
> Am 27.11.19 um 17:29 schrieb Emil Velikov:
> > Hi Thomas,
> > 
> > On Tue, 26 Nov 2019 at 10:15, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> 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>
> >> Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin")
> > 
> > Are you sure the fixes tag is correct? Neither the commit summary nor
> > the patch itself seems related to the changes below.
> 
> Yes, it's correct. It's part of a patch series [1][2][3] that fixes the bug.
> 
> Best regards
> Thomas
> 
> [1]
> https://cgit.freedesktop.org/drm/drm-tip/commit/?id=3a8a5aba142a44eaeba0cb0ec1b4a8f177b5e59a
> [2]
> https://cgit.freedesktop.org/drm/drm-tip/commit/?id=d6d437d97d54c85a1a93967b2745e31dff03365a
> [3]
> https://cgit.freedesktop.org/drm/drm-tip/commit/?id=1591fadf857cdbaf2baa55e421af99a61354713c
> 
I see, different alignment is required for one mga200 GPU.
Personally, I would have mentioned that in the commit message.

Regardless, hats off for the prompt fixup.

Thanks
Emil

[PATCH v2 2/3] drm/mgag200: Store flags from PCI driver data in device structure

[Thomas Zimmermann]

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>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin")
Cc: John Donnelly <john.p.donnelly@oracle.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "José Roberto de Souza" <jose.souza@intel.com>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.3+
---
 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

[PATCH v2 3/3] drm/mgag200: Add workaround for HW that does not support 'startadd'

[Thomas Zimmermann]

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>
Tested-by: John Donnelly <john.p.donnelly@oracle.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Fixes: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin")
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "José Roberto de Souza" <jose.souza@intel.com>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.3+
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

Re: FAILED - [PATCH v2 3/3] drm/mgag200: Add workaround for HW that does not support 'startadd'

[John Donnelly]

Hello Sasha and Thomas ,


This particular patch has failed on one class of servers that has a slightly different Sun Vendor. ID for  the BMC video device: 

I will follow up with additional details in  the review comments for the original message,. 




> On Nov 28, 2019, at 8:23 AM, Sasha Levin <sashal@kernel.org> wrote:
> 
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin").
> 
> The bot has tested the following trees: v5.3.13.
> 
> v5.3.13: Build failed! Errors:
>    drivers/gpu/drm/mgag200/mgag200_drv.c:104:18: error: ‘drm_vram_mm_debugfs_init’ undeclared here (not in a function); did you mean ‘drm_client_debugfs_init’?
> 
> 
> NOTE: The patch will not be queued to stable trees until it is upstream.
> 
> How should we proceed with this patch?
> 
> -- 
> Thanks,
> Sasha
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=t2fPg9D87F7D8jm0_3CG9yoiIKdRg4qc_thBw4bzMhc&m=vxMDOLV77rRe2ekdNFH9IxMSBQrTccltZd8A1H6xYCc&s=efHs2lc_RQYvzLC82c-D3wa8MpX5DCU_YsIo6XruAQg&e=

Re: FAILED - [PATCH v2 3/3] drm/mgag200: Add workaround for HW that does not support 'startadd'

[John Donnelly]

> On Dec 3, 2019, at 11:30 AM, John Donnelly <john.p.donnelly@oracle.com> wrote:
> 
> 
> Hello Sasha and Thomas ,
> 
> 
> This particular patch has failed on one class of servers that has a slightly different Sun Vendor. ID for  the BMC video device: 
> 
> I will follow up with additional details in  the review comments for the original message,. 
> 
> 
> 
> 
>> On Nov 28, 2019, at 8:23 AM, Sasha Levin <sashal@kernel.org> wrote:
>> 
>> Hi,
>> 
>> [This is an automated email]
>> 
>> This commit has been processed because it contains a "Fixes:" tag,
>> fixing commit: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin").
>> 
>> The bot has tested the following trees: v5.3.13.
>> 
>> v5.3.13: Build failed! Errors:
>>   drivers/gpu/drm/mgag200/mgag200_drv.c:104:18: error: ‘drm_vram_mm_debugfs_init’ undeclared here (not in a function); did you mean ‘drm_client_debugfs_init’?
>> 


   I had this same issue and removed that from my local 5.4.0-rc8 build 




>> 
>> NOTE: The patch will not be queued to stable trees until it is upstream.
>> 
>> How should we proceed with this patch?
>> 
>> -- 
>> Thanks,
>> Sasha
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=t2fPg9D87F7D8jm0_3CG9yoiIKdRg4qc_thBw4bzMhc&m=vxMDOLV77rRe2ekdNFH9IxMSBQrTccltZd8A1H6xYCc&s=efHs2lc_RQYvzLC82c-D3wa8MpX5DCU_YsIo6XruAQg&e=
> 

Re: FAILED - [PATCH v2 3/3] drm/mgag200: Add workaround for HW that does not support 'startadd'

[Thomas Zimmermann]

Hi

Am 06.12.19 um 01:48 schrieb John Donnelly:
> 
> 
>> On Dec 3, 2019, at 11:30 AM, John Donnelly <john.p.donnelly@oracle.com> wrote:
>>
>>
>> Hello Sasha and Thomas ,
>>
>>
>> This particular patch has failed on one class of servers that has a slightly different Sun Vendor. ID for  the BMC video device: 
>>
>> I will follow up with additional details in  the review comments for the original message,. 
>>
>>
>>
>>
>>> On Nov 28, 2019, at 8:23 AM, Sasha Levin <sashal@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> [This is an automated email]
>>>
>>> This commit has been processed because it contains a "Fixes:" tag,
>>> fixing commit: 81da87f63a1e ("drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin").
>>>
>>> The bot has tested the following trees: v5.3.13.
>>>
>>> v5.3.13: Build failed! Errors:
>>>   drivers/gpu/drm/mgag200/mgag200_drv.c:104:18: error: ‘drm_vram_mm_debugfs_init’ undeclared here (not in a function); did you mean ‘drm_client_debugfs_init’?
>>>
> 
> 
>    I had this same issue and removed that from my local 5.4.0-rc8 build 

The bot used the wrong tree. The patch has been written against drm-tip.

Best regards
Thomas

> 
> 
> 
> 
>>>
>>> NOTE: The patch will not be queued to stable trees until it is upstream.
>>>
>>> How should we proceed with this patch?
>>>
>>> -- 
>>> Thanks,
>>> Sasha
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=t2fPg9D87F7D8jm0_3CG9yoiIKdRg4qc_thBw4bzMhc&m=vxMDOLV77rRe2ekdNFH9IxMSBQrTccltZd8A1H6xYCc&s=efHs2lc_RQYvzLC82c-D3wa8MpX5DCU_YsIo6XruAQg&e=
>>
> 
> _______________________________________________
> 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