| Message ID | 1424687361-17787-1-git-send-email-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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.. BR, -R > Signed-off-by: Archit Taneja <architt@codeaurora.org> > --- > drivers/gpu/drm/msm/msm_drv.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index a426911..f15494c 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -21,9 +21,11 @@ > > static void msm_fb_output_poll_changed(struct drm_device *dev) > { > +#ifdef CONFIG_DRM_MSM_FBDEV > struct msm_drm_private *priv = dev->dev_private; > if (priv->fbdev) > drm_fb_helper_hotplug_event(priv->fbdev); > +#endif > } > > static const struct drm_mode_config_funcs mode_config_funcs = { > @@ -373,9 +375,11 @@ static void msm_preclose(struct drm_device *dev, struct drm_file *file) > > static void msm_lastclose(struct drm_device *dev) > { > +#ifdef CONFIG_DRM_MSM_FBDEV > struct msm_drm_private *priv = dev->dev_private; > if (priv->fbdev) > drm_fb_helper_restore_fbdev_mode_unlocked(priv->fbdev); > +#endif > } > > static irqreturn_t msm_irq(int irq, void *arg) > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation >
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). 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. -Daniel
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.. > 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.. Plus, I kind of expect phone/tablet/chromebook type stuff would lead the charge into an fbdev-less world.. BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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. -Daniel
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? Archit
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? BR, -R > Archit > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
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
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@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 > 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
On Mon, Mar 09, 2015 at 11:27:06AM +0530, Archit Taneja wrote: > 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@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: > >>>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. See Rob's idea quoted above: Imo you should just do the conditional code in the fbdev emulation helper in drm_fb_helper.c, not in drivers. There's some additional code in i915 (and maybe also msm) to compile out (mostly around taking/dropping console_lock) and we want to keep that. But that should also just be using the same new config option. So: - Add new config option DRM_FBDEV_EMULATION (default y for backwards compat). Use that to stub out helpers in drm_fb_helper.c. - Replace the msm/i915 specific options with the new, i.e. remove it from Kconfig and use DRM_FBDEV_EMULATION in the code instead of the msm/i915 specific one. fbdev-less support for all other drivers will just magically happen without the need for driver-specific code (except for the special locking in i915 and similar things maybe). -Daniel
On 03/09/2015 01:14 PM, Daniel Vetter wrote: > On Mon, Mar 09, 2015 at 11:27:06AM +0530, Archit Taneja wrote: >> 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@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: >>>>> 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. > > See Rob's idea quoted above: Imo you should just do the conditional code > in the fbdev emulation helper in drm_fb_helper.c, not in drivers. There's > some additional code in i915 (and maybe also msm) to compile out (mostly > around taking/dropping console_lock) and we want to keep that. But that > should also just be using the same new config option. So: > > - Add new config option DRM_FBDEV_EMULATION (default y for backwards > compat). Use that to stub out helpers in drm_fb_helper.c. > > - Replace the msm/i915 specific options with the new, i.e. remove it from > Kconfig and use DRM_FBDEV_EMULATION in the code instead of the msm/i915 > specific one. > > fbdev-less support for all other drivers will just magically happen > without the need for driver-specific code (except for the special locking > in i915 and similar things maybe). That sounds good. I wasn't totally sure about fbdev-less support working out of the box on devices that use the fb CMA helpers. The drm_fbdev_cma_* funcs internally use the fb helper functions. Stubbing the fb helpers out might result in some corner cases. However, after a quick look at that the cma helpers, it looks like it take all the necessary precautions to prevent uninitialized accesses. Although, it would be ideal to stub out the drm_fbdev_cma_* helper functions too since they don't serve any purpose when fbdev emulation is disabled. Archit
On Mon, Mar 9, 2015 at 4:54 AM, Archit Taneja <architt@codeaurora.org> wrote: > > > On 03/09/2015 01:14 PM, Daniel Vetter wrote: >> >> On Mon, Mar 09, 2015 at 11:27:06AM +0530, Archit Taneja wrote: >>> >>> 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@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: >>>>>> >>>>>> 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. >> >> >> See Rob's idea quoted above: Imo you should just do the conditional code >> in the fbdev emulation helper in drm_fb_helper.c, not in drivers. There's >> some additional code in i915 (and maybe also msm) to compile out (mostly >> around taking/dropping console_lock) and we want to keep that. But that >> should also just be using the same new config option. So: >> >> - Add new config option DRM_FBDEV_EMULATION (default y for backwards >> compat). Use that to stub out helpers in drm_fb_helper.c. >> >> - Replace the msm/i915 specific options with the new, i.e. remove it from >> Kconfig and use DRM_FBDEV_EMULATION in the code instead of the msm/i915 >> specific one. >> >> fbdev-less support for all other drivers will just magically happen >> without the need for driver-specific code (except for the special locking >> in i915 and similar things maybe). > > > That sounds good. I wasn't totally sure about fbdev-less support working out > of the box on devices that use the fb CMA helpers. The drm_fbdev_cma_* funcs > internally use the fb helper functions. Stubbing the fb helpers out might > result in some corner cases. > > However, after a quick look at that the cma helpers, it looks like it take > all the necessary precautions to prevent uninitialized accesses. Although, > it would be ideal to stub out the drm_fbdev_cma_* helper functions too since > they don't serve any purpose when fbdev emulation is disabled. > I think for a first pass, I wouldn't worry too much about a little unused code in cma fbdev helpers. If we were going for drm tinyification I suspect there are bigger fish to fry ;-) BR, -R > > Archit > > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
On Mon, Mar 09, 2015 at 02:24:09PM +0530, Archit Taneja wrote: > > > On 03/09/2015 01:14 PM, Daniel Vetter wrote: > >On Mon, Mar 09, 2015 at 11:27:06AM +0530, Archit Taneja wrote: > >>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@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: > >>>>>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. > > > >See Rob's idea quoted above: Imo you should just do the conditional code > >in the fbdev emulation helper in drm_fb_helper.c, not in drivers. There's > >some additional code in i915 (and maybe also msm) to compile out (mostly > >around taking/dropping console_lock) and we want to keep that. But that > >should also just be using the same new config option. So: > > > >- Add new config option DRM_FBDEV_EMULATION (default y for backwards > > compat). Use that to stub out helpers in drm_fb_helper.c. > > > >- Replace the msm/i915 specific options with the new, i.e. remove it from > > Kconfig and use DRM_FBDEV_EMULATION in the code instead of the msm/i915 > > specific one. > > > >fbdev-less support for all other drivers will just magically happen > >without the need for driver-specific code (except for the special locking > >in i915 and similar things maybe). > > That sounds good. I wasn't totally sure about fbdev-less support working out > of the box on devices that use the fb CMA helpers. The drm_fbdev_cma_* funcs > internally use the fb helper functions. Stubbing the fb helpers out might > result in some corner cases. > > However, after a quick look at that the cma helpers, it looks like it take > all the necessary precautions to prevent uninitialized accesses. Although, > it would be ideal to stub out the drm_fbdev_cma_* helper functions too since > they don't serve any purpose when fbdev emulation is disabled. I think you could do that as part of the fbdev less work, but in a separate patch. Like Rob says the important part is that we don't set up an fbdev emulation. Taking care of surrounding code like cma (or the special stuff we have in i915) is just icing on the cake. -Daniel
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index a426911..f15494c 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -21,9 +21,11 @@ static void msm_fb_output_poll_changed(struct drm_device *dev) { +#ifdef CONFIG_DRM_MSM_FBDEV struct msm_drm_private *priv = dev->dev_private; if (priv->fbdev) drm_fb_helper_hotplug_event(priv->fbdev); +#endif } static const struct drm_mode_config_funcs mode_config_funcs = { @@ -373,9 +375,11 @@ static void msm_preclose(struct drm_device *dev, struct drm_file *file) static void msm_lastclose(struct drm_device *dev) { +#ifdef CONFIG_DRM_MSM_FBDEV struct msm_drm_private *priv = dev->dev_private; if (priv->fbdev) drm_fb_helper_restore_fbdev_mode_unlocked(priv->fbdev); +#endif } static irqreturn_t msm_irq(int irq, void *arg)
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. Signed-off-by: Archit Taneja <architt@codeaurora.org> --- drivers/gpu/drm/msm/msm_drv.c | 4 ++++ 1 file changed, 4 insertions(+)