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

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

[Archit Taneja]

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(+)

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

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

[Rob Clark]

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
>

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

[Daniel Vetter]

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

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

[Rob Clark]

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

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

[Daniel Vetter]

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

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

[Archit Taneja]

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

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

Re: [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@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

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

[Daniel Vetter]

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

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@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

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

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

[Daniel Vetter]

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

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

[Archit Taneja]

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


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

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

[Rob Clark]

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

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

[Daniel Vetter]

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