Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751942AbdHCKSj (ORCPT ); Thu, 3 Aug 2017 06:18:39 -0400 Received: from mail-it0-f68.google.com ([209.85.214.68]:34765 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751104AbdHCKSi (ORCPT ); Thu, 3 Aug 2017 06:18:38 -0400 MIME-Version: 1.0 X-Originating-IP: [2a02:168:5640:0:960b:2678:e223:c1c6] In-Reply-To: <20170803101119.jwpr7n3bovaqqaz5@phenom.ffwll.local> References: <1501601201-32590-1-git-send-email-david@lechnology.com> <20170802094638.egbninloeganxdrp@phenom.ffwll.local> <8ef4c8cb-59d9-a201-55fc-b7878c6790c7@lechnology.com> <995c3e6d-8920-80b9-d826-7459756fe274@lechnology.com> <20170803101119.jwpr7n3bovaqqaz5@phenom.ffwll.local> From: Daniel Vetter Date: Thu, 3 Aug 2017 12:18:36 +0200 X-Google-Sender-Auth: 6u6lVdsWxNXmRiq2-AKhRnpGK8E Message-ID: Subject: Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev To: David Lechner , dri-devel , Linux Kernel Mailing List , Daniel Vetter Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3326 Lines: 68 On Thu, Aug 3, 2017 at 12:11 PM, Daniel Vetter 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 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 >> > > > >> > > > >> > > > 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