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

[Daniel Vetter]

On Thu, Aug 3, 2017 at 12:11 PM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> > > >
>> > > >
>> > > > 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