Re: [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@xxxxxxxxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > > > > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/