[PATCH] mgag200: some cleanup and a fix for corrupted output

[PATCH] mgag200: some cleanup and a fix for corrupted output

[Christopher Harvey]

Patches 1 to 4 are just cleanup. Maybe these should should be rolled
into one patch?

Patch 5 is a bit more complicated.
On cards with very little video memory, (e.g 8MB) higher resolutions
at 32bit framebuffer depths will get corrupted because the required
memory is larger than what the framebuffer has. DRM has "max_height"
and "max_width" but no "max_bytes" for limiting resolutions, so the
patch is a little hacky. The first for loop tries to associate a
connector with the mode being tested. From the connector I can get the
bpp if the user specified one on the video= commandline. After that I
do 2 things:
 1) Invalidate the requested mode from the video= parameter
 2) Return MODE_BAD if the framebuffer would be too large
I feel this patch plays with structures it shouldn't have to touch to
get the bpp. An alternative fix would be to include a "max_bytes" in
the DRM core and then do these checks there.

I'm also wondering, did I miss the 3.9.0 merge window?

Thanks,

Christopher Harvey (5):
  drm/mgag200: Cleanup: Remove pointless call to drm_fb_get_bpp_depth
  drm/mgag200: Cleanup: 'fbdev_list' in 'struct mga_fbdev' is not used
  drm/mgag200: Cleanup: Pass driver specific mga_device in driver
    functions
  drm/mgag200: Cleanup: Remove extra variable assigns
  drm/mgag200: Reject modes that are too big for VRAM

 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 -
 drivers/gpu/drm/mgag200/mgag200_fb.c   |  3 ---
 drivers/gpu/drm/mgag200/mgag200_main.c |  2 --
 drivers/gpu/drm/mgag200/mgag200_mode.c | 34 ++++++++++++++++++++++++++++++----
 4 files changed, 30 insertions(+), 10 deletions(-)

-- 
1.7.12.4

[PATCH] drm/mgag200: Cleanup: Remove pointless call to drm_fb_get_bpp_depth

[Christopher Harvey]

Signed-off-by: Christopher Harvey <charvey@matrox.com>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 2f48648..d46bd2c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -104,12 +104,9 @@ static int mgag200fb_create_object(struct mga_fbdev *afbdev,
 				   struct drm_gem_object **gobj_p)
 {
 	struct drm_device *dev = afbdev->helper.dev;
-	u32 bpp, depth;
 	u32 size;
 	struct drm_gem_object *gobj;
-
 	int ret = 0;
-	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
 
 	size = mode_cmd->pitches[0] * mode_cmd->height;
 	ret = mgag200_gem_create(dev, size, true, &gobj);
-- 
1.7.12.4

[PATCH] drm/mgag200: Cleanup: 'fbdev_list' in 'struct mga_fbdev' is not used

[Christopher Harvey]

Signed-off-by: Christopher Harvey <charvey@matrox.com>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 5ea5033..4d932c4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -112,7 +112,6 @@ struct mga_framebuffer {
 struct mga_fbdev {
 	struct drm_fb_helper helper;
 	struct mga_framebuffer mfb;
-	struct list_head fbdev_list;
 	void *sysram;
 	int size;
 	struct ttm_bo_kmap_obj mapping;
-- 
1.7.12.4

[PATCH] drm/mgag200: Cleanup: Pass driver specific mga_device in driver functions

[Christopher Harvey]

Signed-off-by: Christopher Harvey <charvey@matrox.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index d3d99a2..3abf197 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1261,9 +1261,8 @@ static const struct drm_crtc_helper_funcs mga_helper_funcs = {
 };
 
 /* CRTC setup */
-static void mga_crtc_init(struct drm_device *dev)
+static void mga_crtc_init(struct mga_device *mdev)
 {
-	struct mga_device *mdev = dev->dev_private;
 	struct mga_crtc *mga_crtc;
 	int i;
 
@@ -1274,7 +1273,7 @@ static void mga_crtc_init(struct drm_device *dev)
 	if (mga_crtc == NULL)
 		return;
 
-	drm_crtc_init(dev, &mga_crtc->base, &mga_crtc_funcs);
+	drm_crtc_init(mdev->dev, &mga_crtc->base, &mga_crtc_funcs);
 
 	drm_mode_crtc_set_gamma_size(&mga_crtc->base, MGAG200_LUT_SIZE);
 	mdev->mode_info.crtc = mga_crtc;
@@ -1502,7 +1501,7 @@ int mgag200_modeset_init(struct mga_device *mdev)
 
 	mdev->dev->mode_config.fb_base = mdev->mc.vram_base;
 
-	mga_crtc_init(mdev->dev);
+	mga_crtc_init(mdev);
 
 	encoder = mga_encoder_init(mdev->dev);
 	if (!encoder) {
-- 
1.7.12.4

[PATCH] drm/mgag200: Cleanup: Remove extra variable assigns

[Christopher Harvey]

These two variables are set again immediately in 'mgag200_modeset_init'

Signed-off-by: Christopher Harvey <charvey@matrox.com>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 70dd3c5..aad280a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -240,8 +240,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 
 	drm_mode_config_init(dev);
 	dev->mode_config.funcs = (void *)&mga_mode_funcs;
-	dev->mode_config.min_width = 0;
-	dev->mode_config.min_height = 0;
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.prefer_shadow = 1;
 
-- 
1.7.12.4

[PATCH] drm/mgag200: Reject modes that are too big for VRAM

[Christopher Harvey]

A monitor or a user could request a resolution greater than the
available VRAM for the backing framebuffer. This change checks the
required framebuffer size against the max VRAM size and rejects modes
if they are too big. This change can also remove a mode request passed
in via the video= parameter.

Signed-off-by: Christopher Harvey <charvey@matrox.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 3abf197..6b5db83 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1405,6 +1405,14 @@ static int mga_vga_get_modes(struct drm_connector *connector)
 static int mga_vga_mode_valid(struct drm_connector *connector,
 				 struct drm_display_mode *mode)
 {
+	struct drm_device *dev = connector->dev;
+	struct mga_device *mdev = (struct mga_device*)dev->dev_private;
+	struct mga_fbdev *mfbdev = mdev->mfbdev;
+	struct drm_fb_helper *fb_helper = &mfbdev->helper;
+	struct drm_fb_helper_connector *fb_helper_conn = NULL;
+	int bpp = 32;
+	int i = 0;
+
 	/* FIXME: Add bandwidth and g200se limitations */
 
 	if (mode->crtc_hdisplay > 2048 || mode->crtc_hsync_start > 4096 ||
@@ -1414,6 +1422,25 @@ static int mga_vga_mode_valid(struct drm_connector *connector,
 		return MODE_BAD;
 	}
 
+	/* Validate the mode input by the user */
+	for (i = 0; i < fb_helper->connector_count; i++) {
+		if (fb_helper->connector_info[i]->connector == connector) {
+			/* Found the helper for this connector */
+			fb_helper_conn = fb_helper->connector_info[i];
+			if (fb_helper_conn->cmdline_mode.specified) {
+				if (fb_helper_conn->cmdline_mode.bpp_specified) {
+					bpp = fb_helper_conn->cmdline_mode.bpp;
+				}
+			}
+		}
+	}
+
+	if ((mode->hdisplay * mode->vdisplay * (bpp/8)) > mdev->mc.vram_size) {
+		if (fb_helper_conn)
+			fb_helper_conn->cmdline_mode.specified = false;
+		return MODE_BAD;
+	}
+
 	return MODE_OK;
 }
 
-- 
1.7.12.4

Re: [PATCH] drm/mgag200: Cleanup: Remove pointless call to drm_fb_get_bpp_depth

[Paul Menzel]

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

Dear Christopher,


thank you for your patches. Not sure if you should CC some maintainer
(or if you did already).


Am Dienstag, den 26.02.2013, 10:53 -0500 schrieb Christopher Harvey:

1. I guess you can remove Cleanup from the commit summary.
2. Why is it »pointless«? I guess a compiler warning that bpp is unused?
Please paste it into the commit message then. As otherwise this cannot
be judged from just looking at the diff below.

> Signed-off-by: Christopher Harvey <charvey@matrox.com>
> ---
>  drivers/gpu/drm/mgag200/mgag200_fb.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
> index 2f48648..d46bd2c 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_fb.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
> @@ -104,12 +104,9 @@ static int mgag200fb_create_object(struct mga_fbdev *afbdev,
>  				   struct drm_gem_object **gobj_p)
>  {
>  	struct drm_device *dev = afbdev->helper.dev;
> -	u32 bpp, depth;
>  	u32 size;
>  	struct drm_gem_object *gobj;
> -
>  	int ret = 0;
> -	drm_fb_get_bpp_depth(mode_cmd->pixel_format, &depth, &bpp);
>  
>  	size = mode_cmd->pitches[0] * mode_cmd->height;
>  	ret = mgag200_gem_create(dev, size, true, &gobj);


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/mgag200: Cleanup: Pass driver specific mga_device in driver functions

[Paul Menzel]

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

Am Dienstag, den 26.02.2013, 10:54 -0500 schrieb Christopher Harvey:
> Signed-off-by: Christopher Harvey <charvey@matrox.com>
> ---
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

[…]

Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/mgag200: Cleanup: Remove extra variable assigns

[Paul Menzel]

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

Am Dienstag, den 26.02.2013, 10:55 -0500 schrieb Christopher Harvey:

*assignments

> These two variables are set again immediately in 'mgag200_modeset_init'

The compiler would optimize this anyway I guess. Though now it can warn
us, if the code should ever change and these variable are not set
anymore in all cases.

> Signed-off-by: Christopher Harvey <charvey@matrox.com>
> ---
>  drivers/gpu/drm/mgag200/mgag200_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 70dd3c5..aad280a 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -240,8 +240,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	drm_mode_config_init(dev);
>  	dev->mode_config.funcs = (void *)&mga_mode_funcs;
> -	dev->mode_config.min_width = 0;
> -	dev->mode_config.min_height = 0;
>  	dev->mode_config.preferred_depth = 24;
>  	dev->mode_config.prefer_shadow = 1;

Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/mgag200: Reject modes that are too big for VRAM

[Paul Menzel]

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

Am Dienstag, den 26.02.2013, 10:55 -0500 schrieb Christopher Harvey:
> A monitor or a user could request a resolution greater than the
> available VRAM for the backing framebuffer. This change checks the
> required framebuffer size against the max VRAM size and rejects modes
> if they are too big. This change can also remove a mode request passed
> in via the video= parameter.
> 
> Signed-off-by: Christopher Harvey <charvey@matrox.com>
> ---
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 3abf197..6b5db83 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1405,6 +1405,14 @@ static int mga_vga_get_modes(struct drm_connector *connector)
>  static int mga_vga_mode_valid(struct drm_connector *connector,
>  				 struct drm_display_mode *mode)
>  {
> +	struct drm_device *dev = connector->dev;
> +	struct mga_device *mdev = (struct mga_device*)dev->dev_private;
> +	struct mga_fbdev *mfbdev = mdev->mfbdev;
> +	struct drm_fb_helper *fb_helper = &mfbdev->helper;
> +	struct drm_fb_helper_connector *fb_helper_conn = NULL;
> +	int bpp = 32;
> +	int i = 0;

It is initialized in the for loop again.

> +
>  	/* FIXME: Add bandwidth and g200se limitations */
>  
>  	if (mode->crtc_hdisplay > 2048 || mode->crtc_hsync_start > 4096 ||
> @@ -1414,6 +1422,25 @@ static int mga_vga_mode_valid(struct drm_connector *connector,
>  		return MODE_BAD;
>  	}
>  
> +	/* Validate the mode input by the user */
> +	for (i = 0; i < fb_helper->connector_count; i++) {
> +		if (fb_helper->connector_info[i]->connector == connector) {
> +			/* Found the helper for this connector */
> +			fb_helper_conn = fb_helper->connector_info[i];
> +			if (fb_helper_conn->cmdline_mode.specified) {
> +				if (fb_helper_conn->cmdline_mode.bpp_specified) {
> +					bpp = fb_helper_conn->cmdline_mode.bpp;
> +				}
> +			}
> +		}
> +	}

Is such a function not used somewhere else already? Like get
user_parameters or so?

> +
> +	if ((mode->hdisplay * mode->vdisplay * (bpp/8)) > mdev->mc.vram_size) {
> +		if (fb_helper_conn)
> +			fb_helper_conn->cmdline_mode.specified = false;

A debug message specifying that this is due to VRAM size would be
helpful I guess.

> +		return MODE_BAD;

I guess that will print the requested mode so it does not need to be
specified above.

> +	}
> +
>  	return MODE_OK;
>  }
>  


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] mgag200: some cleanup and a fix for corrupted output

[Christopher Harvey]

On Tue, Feb 26, 2013 at 10:52:55AM -0500, Christopher Harvey wrote:
> Patches 1 to 4 are just cleanup. Maybe these should should be rolled
> into one patch?
> 
> Patch 5 is a bit more complicated.
> On cards with very little video memory, (e.g 8MB) higher resolutions
> at 32bit framebuffer depths will get corrupted because the required
> memory is larger than what the framebuffer has. DRM has "max_height"
> and "max_width" but no "max_bytes" for limiting resolutions, so the
> patch is a little hacky. The first for loop tries to associate a
> connector with the mode being tested. From the connector I can get the
> bpp if the user specified one on the video= commandline. After that I
> do 2 things:
>  1) Invalidate the requested mode from the video= parameter
>  2) Return MODE_BAD if the framebuffer would be too large
> I feel this patch plays with structures it shouldn't have to touch to
> get the bpp. An alternative fix would be to include a "max_bytes" in
> the DRM core and then do these checks there.
> 
> I'm also wondering, did I miss the 3.9.0 merge window?
> 
> Thanks,
> 
> Christopher Harvey (5):
>   drm/mgag200: Cleanup: Remove pointless call to drm_fb_get_bpp_depth
>   drm/mgag200: Cleanup: 'fbdev_list' in 'struct mga_fbdev' is not used
>   drm/mgag200: Cleanup: Pass driver specific mga_device in driver
>     functions
>   drm/mgag200: Cleanup: Remove extra variable assigns
>   drm/mgag200: Reject modes that are too big for VRAM
> 
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 -
>  drivers/gpu/drm/mgag200/mgag200_fb.c   |  3 ---
>  drivers/gpu/drm/mgag200/mgag200_main.c |  2 --
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 34 ++++++++++++++++++++++++++++++----
>  4 files changed, 30 insertions(+), 10 deletions(-)
> 
> -- 
> 1.7.12.4
> 

Ping.

I've got more patches queuing up. Should I re-submit these along with
the new ones?

So far I've only gotten commit message feedback.

-Chris

Re: [PATCH] mgag200: some cleanup and a fix for corrupted output

[Dave Airlie]

On Thu, Mar 7, 2013 at 4:51 AM, Christopher Harvey <charvey@matrox.com> wrote:
> On Tue, Feb 26, 2013 at 10:52:55AM -0500, Christopher Harvey wrote:
>> Patches 1 to 4 are just cleanup. Maybe these should should be rolled
>> into one patch?
>>
>> Patch 5 is a bit more complicated.
>> On cards with very little video memory, (e.g 8MB) higher resolutions
>> at 32bit framebuffer depths will get corrupted because the required
>> memory is larger than what the framebuffer has. DRM has "max_height"
>> and "max_width" but no "max_bytes" for limiting resolutions, so the
>> patch is a little hacky. The first for loop tries to associate a
>> connector with the mode being tested. From the connector I can get the
>> bpp if the user specified one on the video= commandline. After that I
>> do 2 things:
>>  1) Invalidate the requested mode from the video= parameter
>>  2) Return MODE_BAD if the framebuffer would be too large
>> I feel this patch plays with structures it shouldn't have to touch to
>> get the bpp. An alternative fix would be to include a "max_bytes" in
>> the DRM core and then do these checks there.
>>
>> I'm also wondering, did I miss the 3.9.0 merge window?
>>
>> Thanks,
>>
>> Christopher Harvey (5):
>>   drm/mgag200: Cleanup: Remove pointless call to drm_fb_get_bpp_depth
>>   drm/mgag200: Cleanup: 'fbdev_list' in 'struct mga_fbdev' is not used
>>   drm/mgag200: Cleanup: Pass driver specific mga_device in driver
>>     functions
>>   drm/mgag200: Cleanup: Remove extra variable assigns
>>   drm/mgag200: Reject modes that are too big for VRAM
>>
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 -
>>  drivers/gpu/drm/mgag200/mgag200_fb.c   |  3 ---
>>  drivers/gpu/drm/mgag200/mgag200_main.c |  2 --
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 34 ++++++++++++++++++++++++++++++----
>>  4 files changed, 30 insertions(+), 10 deletions(-)
>>
>> --
>> 1.7.12.4
>>
>
> Ping.
>
> I've got more patches queuing up. Should I re-submit these along with
> the new ones?
>
> So far I've only gotten commit message feedback.
>

Sorry been off sick, and this fell off my radar, I'll get to it today,
the first 4 seem fine,

I'll check the last one today., they all look like fixes so I can
probably merge them at any time.

Thanks,
Dave.

Re: [PATCH] drm/mgag200: Reject modes that are too big for VRAM

[Dave Airlie]

On Wed, Feb 27, 2013 at 1:55 AM, Christopher Harvey <charvey@matrox.com> wrote:
> A monitor or a user could request a resolution greater than the
> available VRAM for the backing framebuffer. This change checks the
> required framebuffer size against the max VRAM size and rejects modes
> if they are too big. This change can also remove a mode request passed
> in via the video= parameter.

I can't say I like the bpp finding code, but my brain is failing to
come up with something better,

so yeah I think this is fine, those cards with 1.5MB VRAM always piss me off :-)

I'll apply these, send on any others!
Dave.