Messages in this thread | | | Subject | Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev | From | David Lechner <> | Date | Wed, 2 Aug 2017 20:37:56 -0500 |
| |
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.
| |