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

[Rob Clark]

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?

BR,
-R

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