drm/fb_helper: prevent some troubles waiting to happen

drm/fb_helper: prevent some troubles waiting to happen

[Ilija Hadzic]

The following two patches address potential problems that I 
called "troubles waiting to happen" in this note
http://lists.freedesktop.org/archives/dri-devel/2011-October/015412.html

I didn't hear anyone take on my question, so I figured I would just send
the patches. I tested the patches on a variety of AMD cards that I have.
I don't have other GPUs handy.

[PATCH 1/2] drm/fb_helper: make sure crtc_count is consistent

[Ilija Hadzic]

stop adding crtcs from dev->mode_config.crtc_list
to crtc_info array if gpu driver specifies (by mistake
or with a reason) fewer crtcs in crtc_count parameter

also, correct crtc_count value if gpu driver
specifies too many crtcs

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_fb_helper.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f7c6854..feac888 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -466,10 +466,18 @@ int drm_fb_helper_init(struct drm_device *dev,
 
 	i = 0;
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		if (i >= crtc_count) {
+			DRM_DEBUG("crtc count set by the gpu reached\n");
+			break;
+		}
 		fb_helper->crtc_info[i].crtc_id = crtc->base.id;
 		fb_helper->crtc_info[i].mode_set.crtc = crtc;
 		i++;
 	}
+	if (i < fb_helper->crtc_count) {
+		DRM_DEBUG("crtc count known by the drm reached\n");
+		fb_helper->crtc_count = i;
+	}
 	fb_helper->conn_limit = max_conn_count;
 	return 0;
 out_free:
-- 
1.7.7

[PATCH 2/2] drm/fb_helper: honor the limit on number of connectors per crtc

[Ilija Hadzic]

gpu driver can specify the limit on the number of connectors
that a given crtc can use. Add a check to make sure this limit
is honored when building a list of connectors associated
with a crtc.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_fb_helper.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index feac888..19e28e9 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1333,6 +1333,11 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 		modeset = &fb_crtc->mode_set;
 
 		if (mode && fb_crtc) {
+			if (modeset->num_connectors >= fb_helper->conn_limit) {
+				DRM_DEBUG("max number of connectors reached for crtc %d\n",
+					  fb_crtc->crtc_id);
+				break;
+			}
 			DRM_DEBUG_KMS("desired mode %s set on crtc %d\n",
 				      mode->name, fb_crtc->mode_set.crtc->base.id);
 			fb_crtc->desired_mode = mode;
-- 
1.7.7

Re: drm/fb_helper: prevent some troubles waiting to happen

[Daniel Vetter]

On Tue, Oct 25, 2011 at 09:57:17PM -0400, Ilija Hadzic wrote:
> The following two patches address potential problems that I 
> called "troubles waiting to happen" in this note
> http://lists.freedesktop.org/archives/dri-devel/2011-October/015412.html
> 
> I didn't hear anyone take on my question, so I figured I would just send
> the patches. I tested the patches on a variety of AMD cards that I have.
> I don't have other GPUs handy.

I've quickly checked current callsites of drm_fb_helper_init and I think
you can just kill the two arguments crtc_count and max_conn_count. All
drivers pass in the actual crtc count for the first and some midly bogus
constant for the latter.

i915 and radeon also use that constant to attach some struct drm_connector
* pointers to the end of their crtc, which looks rather unused. You might
want to kill that, too.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: drm/fb_helper: prevent some troubles waiting to happen

[Ilija Hadzic]

On Wed, 26 Oct 2011, Daniel Vetter wrote:

>
> I've quickly checked current callsites of drm_fb_helper_init and I think
> you can just kill the two arguments crtc_count and max_conn_count.

It is a usable functionality (i.e. allows the driver to select which 
connectors or CRTCs is fbcon allowed to bind to) and I see value in it.
I am actually doing some work (not ready for public release yet, but will 
be in some relatively near future) that makes the use of this feature.

If you (and the rest of the community) would prefer to see the use case 
first before merging these two patches, I am perfectly fine with waiting 
(I'll resend later, after I show the use case), but I would not like to 
see the functionality killed only because drivers don't use it at present.

thanks,

Ilija