Re: [RFC 1/6] drm: Add top level Kconfig option for DRM fbdev emulation

[Daniel Vetter]

Any updates on this or too much distractions? I really think doing
this would be awesome for the drm subsystem, instead of reinventing
this wheel for each driver.
-Daniel

On Wed, Mar 25, 2015 at 10:21 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Wed, Mar 25, 2015 at 01:47:54PM +0530, Archit Taneja wrote:
>> Hi,
>>
>> On 03/13/2015 02:36 PM, Daniel Vetter wrote:
>> >On Fri, Mar 13, 2015 at 11:55:07AM +0530, Archit Taneja wrote:
>> >>
>> >>
>> >>On 03/11/2015 08:47 PM, Daniel Vetter wrote:
>> >>>On Wed, Mar 11, 2015 at 01:51:02PM +0530, Archit Taneja wrote:
>> >>>>
>> >>>>
>> >>>>On 03/10/2015 05:47 PM, Daniel Vetter wrote:
>> >>>>>On Tue, Mar 10, 2015 at 03:52:41PM +0530, Archit Taneja wrote:
>> >>>>>>On 03/10/2015 03:35 PM, Daniel Vetter wrote:
>> >>>>>>>On Tue, Mar 10, 2015 at 03:22:49PM +0530, Archit Taneja wrote:
>> >>>>>>>>On 03/10/2015 03:17 PM, Daniel Vetter wrote:
>> >>>>>>>>>On Tue, Mar 10, 2015 at 03:11:28PM +0530, Archit Taneja wrote:
>> >>>>>>>>>>diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> >>>>>>>>>>index 151a050..38f83a0 100644
>> >>>>>>>>>>--- a/drivers/gpu/drm/Kconfig
>> >>>>>>>>>>+++ b/drivers/gpu/drm/Kconfig
>> >>>>>>>>>>@@ -40,6 +40,24 @@ config DRM_KMS_FB_HELPER
>> >>>>>>>>>>    help
>> >>>>>>>>>>      FBDEV helpers for KMS drivers.
>> >>>>>>>>>>
>> >>>>>>>>>>+config DRM_FBDEV_EMULATION
>> >>>>>>>>>>+   bool "Enable legacy fbdev support for your modesetting driver"
>> >>>>>>>>>>+   depends on DRM
>> >>>>>>>>>>+   select DRM_KMS_HELPER
>> >>>>>>>>>>+   select DRM_KMS_FB_HELPER
>> >>>>>>>>>>+   select FB_SYS_FILLRECT
>> >>>>>>>>>>+   select FB_SYS_COPYAREA
>> >>>>>>>>>>+   select FB_SYS_IMAGEBLIT
>> >>>>>>>>>>+   select FB_SYS_FOPS
>> >>>>>>>>>>+   select FB_CFB_FILLRECT
>> >>>>>>>>>>+   select FB_CFB_COPYAREA
>> >>>>>>>>>>+   select FB_CFB_IMAGEBLIT
>> >>>>>>>>>>+   default y
>> >>>>>>>>>>+   help
>> >>>>>>>>>>+     Choose this option if you have a need for the legacy fbdev
>> >>>>>>>>>>+     support. Note that this support also provide the linux console
>> >>>>>>>>>>+     support on top of your modesetting driver.
>> >>>>>>>>>
>> >>>>>>>>>Maybe clarify that for linux console support you also need
>> >>>>>>>>>CONFIG_FRAMEBUFFER_CONSOLE? fbdev alone isn't enough.
>> >>>>>>>>
>> >>>>>>>>DRM_KMS_FB_HELPER selects that for us, right?
>> >>>>>>>
>> >>>>>>>Hm right I've missed that. Reminds me that you need one more patch at the
>> >>>>>>>end to remove all the various select DRM_KMS_FB_HELPER from all drm
>> >>>>>>>drivers. Otherwise this knob here won't work by default if you e.g. select
>> >>>>>>>radeon. In general we can't mix explicit options with menu entries with a
>> >>>>>>>select.
>> >>>>>>
>> >>>>>>I was trying that out. Removing DRM_KMS_FB_HELPER and having
>> >>>>>>DRM_FBDEV_EMULATION disabled breaks drivers which use FB stuff internally in
>> >>>>>>their respective xyz_fbdev.c files.
>> >>>>>
>> >>>>>But with the stubbed out functions that should work, right? Why doesn't
>> >>>>>it?
>> >>>>
>> >>>>There are still calls to functions from fb core like fb_set_suspend and
>> >>>>register_framebuffer which aren't covered by the drm fb helper functions.
>> >>>
>> >>>Hm, sounds like we need another patch to stub out fb_set_suspend when
>> >>>fbdev isn't enabled. Is there anything else?
>> >>
>> >>There are a handful of fb core functions which are called by drm drivers:
>> >>
>> >>fb_alloc_cmap/fb_dealloc_cmap
>> >>
>> >>fb_sys_read/fb_sys_write
>> >>
>> >>register_framebuffer/unregister_framebuffer/unlink_framebuffer/
>> >>remove_conflicting_framebuffers
>> >>
>> >>fb_set_suspend
>> >>
>> >>fb_deferred_io_init/fb_deferred_io_cleanup
>> >>
>> >>framebuffer_alloc/framebuffer_release
>> >
>> >Hm yeah that's somewhat annoying indeed. What about the following:
>> >1. We move all the #include <linux/fb.h> from drivers into drm_fb_helper.h
>> >
>> >2. Then we add stubs for these functions in drm_fb_helper.h, like this
>> >
>> >#if defined(CONFIG_FB)
>> >#include <linux/fb.h>
>> >#else
>> >
>> >/* static inline stubs for all the fb stuff used by kms drivers */
>> >#endif
>> >
>> >Imo this makes sense since kms drivers really have a bit a special
>> >situation with fbdev. They're not full-blown fbdev drivers and can be
>> >useful fully without fbdev.
>> >
>>
>> I was trying this out. Removing 'linux/fb.h' and replacing stub fb funcs
>> won't really work because struct declarations(like fb_info) also get
>> removed.
>>
>> I considered placing '#if IS_ENABLED(CONFIG_FB)' within linux/fb.h itself,
>> but that seemed a bit too intrusive.
>>
>> This is what I'm currently doing:
>>
>> - Some funcs, like framebufer_alloc/release, alloc_cmap/dealloc_cmap would
>> actually benefit if we have drm fb helpers for them. They are used in
>> exactly the same manner by all the drivers.
>>
>> - For the rest of the functions that are sparsely used, I was considering
>> making very simple drm_fb_* wrapper functions. Something like:
>>
>> void drm_fb_helper_deferred_io_init(struct drm_fb_helper *helper)
>> {
>>       if (helper->fbdev)
>>               fb_deferred_io_init(helper->fbdev);
>> }
>>
>> We could have all fb calls called within drm_fb_helper.c, creating
>> drm_fb_helper_* stub functions would then be an easier task. What do you
>> think?
>
> That's actually an option I considered, but I hoped we could do it with
> less work. If this indeed works and you can create the patch that would be
> awesome.
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/