Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752324AbdHCKL3 (ORCPT ); Thu, 3 Aug 2017 06:11:29 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:37100 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752041AbdHCKLZ (ORCPT ); Thu, 3 Aug 2017 06:11:25 -0400 Date: Thu, 3 Aug 2017 12:11:19 +0200 From: Daniel Vetter To: David Lechner Cc: Daniel Vetter , dri-devel , Linux Kernel Mailing List , Daniel Vetter Subject: Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev Message-ID: <20170803101119.jwpr7n3bovaqqaz5@phenom.ffwll.local> Mail-Followup-To: David Lechner , dri-devel , Linux Kernel Mailing List , Daniel Vetter 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <995c3e6d-8920-80b9-d826-7459756fe274@lechnology.com> X-Operating-System: Linux phenom 4.11.0-2-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3343 Lines: 79 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(). > > > > > 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