Message-ID: <20170803101119.jwpr7n3bovaqqaz5@phenom.ffwll.local>
Date:   Thu, 3 Aug 2017 12:11:19 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     David Lechner <david@...hnology.com>
Cc:     Daniel Vetter <daniel@...ll.ch>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev

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

Powered by blists - more mailing lists