Re: [PATCH RESEND v2 06/25] drm/fb_helper: Create a wrapper for remove_conflicting_framebuffers

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 29, 2015 at 09:49:04AM +0530, Archit Taneja wrote:
> 
> 
> On 07/29/2015 12:16 AM, Daniel Vetter wrote:
> >On Wed, Jul 22, 2015 at 02:58:01PM +0530, Archit Taneja wrote:
> >>Some drm drivers call remove_conflicting_framebuffers. Create a
> >>drm_fb_helper function that wraps around these calls.
> >>
> >>This is part of an effort to prevent drm drivers from calling fbdev
> >>functions directly, in order to make fbdev emulation a top level drm
> >>option.
> >>
> >>v2:
> >>- Added kerneldocs
> >>- Follow the drm way of aligning of arguments in func definitions
> >>
> >>Signed-off-by: Archit Taneja <architt@codeaurora.org>
> >>---
> >>  drivers/gpu/drm/drm_fb_helper.c | 15 +++++++++++++++
> >>  include/drm/drm_fb_helper.h     |  4 ++++
> >>  2 files changed, 19 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>index 9620aa5..86e4e2c 100644
> >>--- a/drivers/gpu/drm/drm_fb_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_helper.c
> >>@@ -894,6 +894,21 @@ void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper, int state)
> >>  }
> >>  EXPORT_SYMBOL(drm_fb_helper_set_suspend);
> >>
> >>+/**
> >>+ * drm_fb_helper_remove_conflicting_framebuffers - wrapper around
> >>+ *						remove_conflicting_framebuffers
> >>+ * @fb_helper: driver-allocated fbdev helper
> >>+ *
> >>+ * A wrapper around remove_conflicting_framebuffers implemented by fbdev core
> >>+ */
> >>+int drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a,
> >>+						  const char *name,
> >>+						  bool primary)
> >>+{
> >>+	return remove_conflicting_framebuffers(a, name, primary);
> >>+}
> >>+EXPORT_SYMBOL(drm_fb_helper_remove_conflicting_framebuffers);
> >
> >Chris Wilson reported on irc that if he builds with I915_FBDEV=n then
> >there's a missing symbol problem here because i915 wants to kick out other
> >framebuffers (well we have to for correctness) even when fbdev emulation
> >is disabled. If you look at i915_dma.c you'll see that the call to
> >remove_conflicting_framebuffers is conditional upon CONFIG_FB and not
> >CONFIG_I915_FBDEV.
> >
> >I think the proper solution here would be to provide a static inline
> >helper for remove_conflicting_framebuffer itself when CONFIG_FB=n, since a
> >all drivers who call this still want to call this even when fbdev
> >emulation is disabled.
> >
> >Plan B would be to move this into core drm and make the static inline one
> >selected for CONFIG_FB=n. If you do that then maybe also rename it to
> >drm_remove_conflicting_framebuffer since this really is only tangetial to
> >fbdev emulation.
> 
> The remove_conflicting_framebuffers calls do_unregister_framebuffer(),
> which seems to be quite central to fb core, and can't just work without
> other fb core funcs.
> 
> What I don't get is, if legacy fb was never enabled, how did we end up
> registering fbs in the first place?
> 
> Could you briefly explain what these other 'framebuffers' are that
> remove_conflicting_framebuffers removes? The comments say that they are
> firmware allocated fbs. I'm not well versed with what happens in
> desktop graphics.

Following config is the relevant one:

CONFIG_FB=y
CONFIG_DRM_I915_FBDEV=n

I.e. core fbdev is enabled and so we might load vesafb or efifb and
similar firmware fbdev drivers. But drm/i915 fbdev emulation isn't
enabled. Still we absolutely must kick off these firmware fbs in that
case.
-Daniel

> 
> Archit
> 
> >
> >Anyway current patch unfortunately won't work since we really must remove
> >other framebuffers even in the FB=y, I915_FBDEV=n case. So I dropped it
> >from drm-misc for now. Unfortunately that means all the driver conversions
> >won't apply anymore either :(
> >
> >Thanks, Daniel
> >>+
> >>  static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
> >>  		     u16 blue, u16 regno, struct fb_info *info)
> >>  {
> >>diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> >>index ef32500..cbdc69d 100644
> >>--- a/include/drm/drm_fb_helper.h
> >>+++ b/include/drm/drm_fb_helper.h
> >>@@ -168,6 +168,10 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> >>
> >>  void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper, int state);
> >>
> >>+int drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a,
> >>+						  const char *name,
> >>+						  bool primary);
> >>+
> >>  int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info);
> >>
> >>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
> >>--
> >>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >>hosted by The Linux Foundation
> >>
> >
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel