Subject: Re: [PATCH] drm/fb-helper: pass physical dimensions to fbdev
To: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Daniel Vetter <daniel.vetter@intel.com>
References: <1501601201-32590-1-git-send-email-david@lechnology.com>
 <20170802094638.egbninloeganxdrp@phenom.ffwll.local>
 <8ef4c8cb-59d9-a201-55fc-b7878c6790c7@lechnology.com>
 <CAKMK7uFotPC98xEYUQJ2O7m=PtWqurku1HqFpEzwzppJZD=5tQ@mail.gmail.com>
From: David Lechner <david@lechnology.com>
Message-ID: <995c3e6d-8920-80b9-d826-7459756fe274@lechnology.com>
Date: Wed, 2 Aug 2017 20:37:56 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <CAKMK7uFotPC98xEYUQJ2O7m=PtWqurku1HqFpEzwzppJZD=5tQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2536
Lines: 63

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.