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

[Daniel Vetter <daniel@ffwll.ch>]

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@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@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@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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch