drm/fb-helper: pass physical dimensions to fbdev

The fbdev subsystem has a place for physical dimensions (width and height in mm) that is readable by userspace. Since DRM also knows these dimensions, pass this information to the fbdev device.

Signed-off-by David Lechner <[email protected]>

---
 drivers/gpu/drm/drm_fb_helper.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/gpu/drm/drm_fb_helper.c +13 -0

index 574af01..07a6621 100644
@@ -1759,6 +1759,7 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
 			    uint32_t fb_width, uint32_t fb_height)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
+	int i;
 
 	info->pseudo_palette = fb_helper->pseudo_palette;
 	info->var.xres_virtual = fb->width;

@@ -1771,6 +1772,18 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
 	info->var.height = -1;
 	info->var.width = -1;
 
+	drm_fb_helper_for_each_connector(fb_helper, i) {
+		struct drm_connector *connector =
+			fb_helper->connector_info[i]->connector;
+
+		/* use the first connected connector for the physical dimensions */
+		if (connector->status == connector_status_connected) {
+			info->var.height = connector->display_info.width_mm;
+			info->var.width = connector->display_info.height_mm;
+			break;
+		}
+	}
+
 	switch (fb->format->depth) {
 	case 8:
 		info->var.red.offset = 0;

Signature · 6 lines

2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 5 lines · David Lechner

The fbdev subsystem has a place for physical dimensions (width and height in mm) that is readable by userspace. Since DRM also knows these dimensions, pass this information to the fbdev device.

Signed-off-by David Lechner <[email protected]>

Still in the wrong function. Also please add some notation about what you changed when resubmitting a patch (it took me a while to remember that I replied to you already). That makes patch reviewing more efficient.

Quoted 7 lines · Daniel Vetter

Quoted 5 lines · David Lechner

The fbdev subsystem has a place for physical dimensions (width and height in mm) that is readable by userspace. Since DRM also knows these dimensions, pass this information to the fbdev device.

Quoted 7 lines

Quoted 1 line

Signed-off-by David Lechner <[email protected]>

Still in the wrong function. Also please add some notation about what you changed when resubmitting a patch (it took me a while to remember that I replied to you already). That makes patch reviewing more efficient.

Sorry for being so dense. :-/

I did read your first reply at least 10 times. All of the terminology is foreign to me, but after sleeping on it a few days, I think it is slowly soaking into my brain.

Quoted 16 lines · David Lechner

Quoted 7 lines · Daniel Vetter

Quoted 5 lines · David Lechner

The fbdev subsystem has a place for physical dimensions (width and height in mm) that is readable by userspace. Since DRM also knows these dimensions, pass this information to the fbdev device.

Quoted 7 lines

Quoted 1 line

Signed-off-by David Lechner <[email protected]>

Still in the wrong function. Also please add some notation about what you changed when resubmitting a patch (it took me a while to remember that I replied to you already). That makes patch reviewing more efficient.

Quoted 5 lines

Sorry for being so dense. :-/

I did read your first reply at least 10 times. All of the terminology is foreign to me, but after sleeping on it a few days, I think it is slowly soaking into my brain.

No problem, the code is fairly convoluted. One more question on your
v3: From reading fbdev code I don't see any place that overwrites the physical dimensions except the fill_var helper function (which is called deep down from register_framebuffer). If we entirely remove the var.width/height assignments from that (including the -1 default) and move all of it into setup_crtcs, would that work?

I kinda don't like have the same logic in 2 completely different places, once for driver load and once for hotplug handling. That tends to cause bugs (because then no one bothers to test hotplug handling or the boot-up case properly).

Quoted 23 lines · Daniel Vetter

Quoted 16 lines · David Lechner

Quoted 7 lines · Daniel Vetter

Quoted 5 lines · David Lechner

The fbdev subsystem has a place for physical dimensions (width and height in mm) that is readable by userspace. Since DRM also knows these dimensions, pass this information to the fbdev device.

Quoted 7 lines

Quoted 1 line

Signed-off-by David Lechner <[email protected]>

Still in the wrong function. Also please add some notation about what you changed when resubmitting a patch (it took me a while to remember that I replied to you already). That makes patch reviewing more efficient.

Quoted 5 lines

Sorry for being so dense. :-/

I did read your first reply at least 10 times. All of the terminology is foreign to me, but after sleeping on it a few days, I think it is slowly soaking into my brain.

Quoted 6 lines

No problem, the code is fairly convoluted. One more question on your
v3: From reading fbdev code I don't see any place that overwrites the physical dimensions except the fill_var helper function (which is called deep down from register_framebuffer). If we entirely remove the var.width/height assignments from that (including the -1 default) and move all of it into setup_crtcs, would that work?

I tried that, but in my case, drm_setup_crtcs() is only called once before the fbdev is allocated. So, I got a kernel oops from dereferencing a null pointer, which is why there is a null check for fb_helper->fbdev as part of the if statement I added there. Since drm_setup_crtcs() is not called again, the var.width and height are never set.

I also noticed that there is another similar workaround/code duplication for the framebuffer not being allocated in the first call to drm_setup_crtcs() seen at the end of drm_fb_helper_single_fb_probe().

Quoted 6 lines

I kinda don't like have the same logic in 2 completely different places, once for driver load and once for hotplug handling. That tends to cause bugs (because then no one bothers to test hotplug handling or the boot-up case properly).

Would it be possible to split drm_setup_crtcs() into two functions, a top half and a bottom half. During init (pseudo-code)...

drm_begin_setup_crtcs()
drm_fb_helper_single_fb_probe()
drm_finish_setup_crtcs()

and during hotplug event, just...

drm_begin_setup_crtcs()
drm_finish_crtcs_bottom_half()

I think it could solve both cases.

Quoted 32 lines · David Lechner

Quoted 23 lines · Daniel Vetter

Quoted 16 lines · David Lechner

Quoted 7 lines · Daniel Vetter

Quoted 5 lines · David Lechner

The fbdev subsystem has a place for physical dimensions (width and height in mm) that is readable by userspace. Since DRM also knows these dimensions, pass this information to the fbdev device.

Quoted 7 lines

Quoted 1 line

Signed-off-by David Lechner <[email protected]>

Still in the wrong function. Also please add some notation about what you changed when resubmitting a patch (it took me a while to remember that I replied to you already). That makes patch reviewing more efficient.

Quoted 5 lines

Sorry for being so dense. :-/

I did read your first reply at least 10 times. All of the terminology is foreign to me, but after sleeping on it a few days, I think it is slowly soaking into my brain.

Quoted 6 lines

No problem, the code is fairly convoluted. One more question on your
v3: From reading fbdev code I don't see any place that overwrites the physical dimensions except the fill_var helper function (which is called deep down from register_framebuffer). If we entirely remove the var.width/height assignments from that (including the -1 default) and move all of it into setup_crtcs, would that work?

Quoted 5 lines

I tried that, but in my case, drm_setup_crtcs() is only called once before the fbdev is allocated. So, I got a kernel oops from dereferencing a null pointer, which is why there is a null check for fb_helper->fbdev as part of the if statement I added there. Since drm_setup_crtcs() is not called again, the var.width and height are never set.

fb_helper->fbdev is allocated in drm_fb_helper_alloc_fbi, which /should/ be called as the very first thing in setting up the entire fbdev stuff. If you're oopsing on that, something went wrong. We also set info->var in a few other places before calling register_framebuffer already (see the line that sets info->var.pixclock right before calling register_framebuffer).

Quoted 3 lines

I also noticed that there is another similar workaround/code duplication for the framebuffer not being allocated in the first call to drm_setup_crtcs() seen at the end of drm_fb_helper_single_fb_probe().