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(-)
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);
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
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;
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) {
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
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;
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
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; }
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
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.
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:
- Invalidate the requested mode from the video= parameter
- 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
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:
- Invalidate the requested mode from the video= parameter
- 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.
dri-devel@lists.freedesktop.org
-
Christopher Harvey -
Dave Airlie -
Paul Menzel