[PATCH] drm/fb-helper: pass physical dimensions to fbdev

[PATCH] drm/fb-helper: pass physical dimensions to fbdev

[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 <david@lechnology.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 574af01..07a6621 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -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;
-- 
2.7.4

Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev

[Daniel Vetter]

On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote:
> 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 <david@lechnology.com>

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.

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 574af01..07a6621 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -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;
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev

[David Lechner]

On 08/02/2017 04:46 AM, Daniel Vetter wrote:
> On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote:
>> 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 <david@lechnology.com>
> 
> 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.

Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev

[Daniel Vetter]

On Wed, Aug 2, 2017 at 6:37 PM, David Lechner <david@lechnology.com> wrote:
> On 08/02/2017 04:46 AM, Daniel Vetter wrote:
>>
>> On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote:
>>>
>>> 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 <david@lechnology.com>
>>
>>
>> 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.

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).

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev

[David Lechner]

On 08/02/2017 06:28 PM, Daniel Vetter wrote:
> On Wed, Aug 2, 2017 at 6:37 PM, David Lechner <david@lechnology.com> wrote:
>> On 08/02/2017 04:46 AM, Daniel Vetter wrote:
>>>
>>> On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote:
>>>>
>>>> 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 <david@lechnology.com>
>>>
>>>
>>> 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.
> 
> 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().

> 
> 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.

Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev

[Daniel Vetter]

On Wed, Aug 02, 2017 at 08:37:56PM -0500, David Lechner wrote:
> On 08/02/2017 06:28 PM, Daniel Vetter wrote:
> > On Wed, Aug 2, 2017 at 6:37 PM, David Lechner <david@lechnology.com> wrote:
> > > On 08/02/2017 04:46 AM, Daniel Vetter wrote:
> > > > 
> > > > On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote:
> > > > > 
> > > > > 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 <david@lechnology.com>
> > > > 
> > > > 
> > > > 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.
> > 
> > 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.

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).

> 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().
> 
> > 
> > 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.

I'm still not following where exactly and how you're blowing up. Can pls
paste the backtrace of how that all blows up?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev

[Daniel Vetter]

On Thu, Aug 3, 2017 at 12:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 02, 2017 at 08:37:56PM -0500, David Lechner wrote:
>> On 08/02/2017 06:28 PM, Daniel Vetter wrote:
>> > On Wed, Aug 2, 2017 at 6:37 PM, David Lechner <david@lechnology.com> wrote:
>> > > On 08/02/2017 04:46 AM, Daniel Vetter wrote:
>> > > >
>> > > > On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote:
>> > > > >
>> > > > > 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 <david@lechnology.com>
>> > > >
>> > > >
>> > > > 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.
>> >
>> > 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.
>
> 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).
>
>> 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().

Ok I screwed up, alloc_fbi is called from fb_probe, which is called
only from helper_single_probe(). Sorry about all my confusion, sounds
like we indeed have to duplicate this. Since the duplication is
necessary, can you pls extract an update_var static helper function,
which is called from both places? I'd put the call in
drm_fb_helper_hotplugt_event
and right before the register_framebuffer for the load path, not
trying to be clever like I suggested and hiding it somewhere in
drm_setup_crtcs.

Sorry for going all over the place with this, but fbdev isn't really
something I know all that well ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch