[PATCH] drm: msm: Fix build when legacy fbdev support isn't set

[Archit Taneja]

On 03/05/2015 09:14 PM, Daniel Vetter wrote:
> On Thu, Mar 05, 2015 at 07:10:44AM -0500, Rob Clark wrote:
>> On Thu, Mar 5, 2015 at 5:06 AM, Archit Taneja <architt at codeaurora.org> wrote:
>>>
>>> On 02/23/2015 09:09 PM, Daniel Vetter wrote:
>>>>
>>>> On Mon, Feb 23, 2015 at 10:03:21AM -0500, Rob Clark wrote:
>>>>>
>>>>> On Mon, Feb 23, 2015 at 9:09 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
>>>>>>
>>>>>> On Mon, Feb 23, 2015 at 08:33:36AM -0500, Rob Clark wrote:
>>>>>>>
>>>>>>> On Mon, Feb 23, 2015 at 5:29 AM, Archit Taneja <architt at codeaurora.org>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> The DRM_KMS_FB_HELPER config is selected only when DRM_MSM_FBDEV
>>>>>>>> config is
>>>>>>>> selected. The driver accesses drm_fb_helper_* functions even when
>>>>>>>> legacy fbdev
>>>>>>>> support is disabled in msm. Wrap around these functions with #ifdef
>>>>>>>> checks to
>>>>>>>> prevent build break.
>>>>>>>
>>>>>>>
>>>>>>> hmm, perhaps rather than solving this in each driver, we should do
>>>>>>> some stub versions of those fb-helper fxns?
>>>>>>>
>>>>>>> There are at least one or two other drivers that can build without
>>>>>>> fbdev, and I guess more going forward..
>>>>>>
>>>>>>
>>>>>> It's not quite that easy since you also have to start/stop the vt
>>>>>> subsystem at the right point in time in your own driver. See
>>>>>> intel_fbdev_set_suspend. If you don't do that there's no synchronization
>>>>>> between fbcon shutting down/resuming and your driver, which in the best
>>>>>> case means fbcon does some writes to nowhere and worst case means your
>>>>>> chip dies (mmio to offline chip blocks) or writes go to somewhere random
>>>>>> in system memory (iommu contains some stale ptes since not yet fully
>>>>>> restored, more an issue with hibernate).
>>>>>
>>>>>
>>>>> I guess I don't fully follow the vt/fbcon interaction if there is no
>>>>> fbdev driver...  but then again I don't have vesafb/efifb to contend
>>>>> with, so I'm assuming something to do with that..
>>>>
>>>>
>>>> It's the other way round: There's interaction when we have fbdev enabled
>>>> beyond just calling a few fbdev helper functions. And we should compile
>>>> that out too since the console_lock is way too evil ;-)
>>>>
>>>> Only with these additional #ifdefs is i915 completely console_lock free if
>>>> you disable i915 fbdev support. Just stubbing out the fbdev helper
>>>> functions is not enough.
>>>>
>>>>>> And because the console_lock is massively contended we do that in a
>>>>>> async
>>>>>> worker in i915.
>>>>>>
>>>>>> But anyway I agree it would still simply drivers quite a bit if we'd
>>>>>> have
>>>>>> support for dummy fb helpers in the core, maybe with an explicit
>>>>>> Kconfig.
>>>>>> Then drivers could switch to using that for the additional #ifdef (like
>>>>>> the vt stuff i915 does) and otherwise rely upon dummy static inline.
>>>>>> That
>>>>>> would give us fbdev-less support for most drivers for free, which is
>>>>>> kinda
>>>>>> neat.
>>>>>
>>>>>
>>>>> I guess at least for all the arm drivers, life without fbdev doesn't
>>>>> have these extra complications, so at least they could use stubs..
>>>>
>>>>
>>>> Does the problem sound more tricky with the above clarification? If you
>>>> don't do the fb_set_suspend call then I expect you'll have some
>>>> interesting problems.
>>>>
>>>>> Plus, I kind of expect phone/tablet/chromebook type stuff would lead
>>>>> the charge into an fbdev-less world..
>>>>
>>>>
>>>> Yeah, that's another reason to support fbdev-less in the helpers instead
>>>> of each driver.
>>>
>>>
>>> I was trying to create a patch with the idea above. This works well if we
>>> want the kernel to support only one DRM driver. If the kernel supports
>>> multiple platforms and one DRM driver sets its config to enable legacy fbdev
>>> and another doesn't, we still end up building the fbdev helper funcs.
>>> Drivers built without legacy fbdev would need to be very strict(check for
>>> priv->fbdev not NULL) to prevent calling them.
>>>
>>> The fb cma helper also adds to the difficulties. The cma helper seems to
>>> have some functions that provide legacy fbdev support and others that allow
>>> allocation of drm_framebuffers and gem objects. We'd need to be careful
>>> about our stub functions not messing up the drivers using the fb cma
>>> helpers.
>>>
>>> Rather than creating fb helper stub functions, maybe we could help each drm
>>> driver create two variants of functions needed by drm core(like
>>> output_poll_changed and dev_lastclose), one variant supporting legacy fbdev,
>>> and the other not?
>>
>> So one quick thought.. building without fbdev would ideally/eventually
>> be a distro level decision, not a driver level decision..  so I think
>> it is *eventually* not a problem for it being a global drm level
>> decision.  The only problem is right now some drivers support no-fbdev
>> config, and some do not.  Maybe it is worth fixing that?
>
> Yeah, if we get fbdev emulation Kconfig option then I think i915 and msm
> should remove their own options and just use that. There's really no need
> to have this per-driver, this is a question of what userspace expects and
> so per-distro, independant of the driver.
> -Daniel
>

Okay. If I understand right. We need to do something like this:

- Create a new Kconfig option that lets us emulate fbdev

- For drivers that already have a config for fbdev emulation, replace it 
with our new emulation config.

- For drivers that assume fbdev emulation by default, select our new 
emulation config in their respective Kconfigs.

Does this sound okay?

I suppose this could be the first step. Later we'd need to work on each 
driver to work with and without the fbdev emulation Kconfig option.

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project