Re: [RFC v2 6/6] drm/i915: Remove local fbdev emulation Kconfig option

From: Daniel Vetter <daniel@ffwll.ch>
To: Archit Taneja <architt@codeaurora.org>
Cc: dri-devel@lists.freedesktop.org, daniel@ffwll.ch,
	linux-arm-msm@vger.kernel.org, airlied@linux.ie,
	treding@nvidia.com, p.zabel@pengutronix.de,
	benjamin.gaignard@linaro.org, jani.nikula@linux.intel.com
Subject: Re: [RFC v2 6/6] drm/i915: Remove local fbdev emulation Kconfig option
Date: Mon, 13 Jul 2015 09:10:01 +0200	[thread overview]
Message-ID: <20150713071001.GJ3736@phenom.ffwll.local> (raw)
In-Reply-To: <1436769731-4816-7-git-send-email-architt@codeaurora.org>

On Mon, Jul 13, 2015 at 12:12:11PM +0530, Archit Taneja wrote:
> DRM_I915_FBDEV config is currently used to enable/disable fbdev emulation
> for the i915 kms driver.
> 
> Replace this with the top level DRM_FBDEV_EMULATION config option. Using
> this config lets us also prevent wrapping around drm_fb_helper_* calls with
> 
> The #ifdef in drm_i915_private struct adding/removing members intel_fbdev
> and fbdev_suspend_work has been removed. This helps us use stub drm helper
> functions at not much cost.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/i915/Kconfig         | 15 ---------------
>  drivers/gpu/drm/i915/Makefile        |  4 ++--
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |  2 --
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++------
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 14 ++++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +--
>  drivers/gpu/drm/i915/intel_fbdev.c   |  7 -------
>  8 files changed, 16 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 74acca9..bd9ccfc 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -45,21 +45,6 @@ config DRM_I915_KMS
>  
>  	  If in doubt, say "Y".
>  
> -config DRM_I915_FBDEV
> -	bool "Enable legacy fbdev support for the modesetting intel driver"
> -	depends on DRM_I915
> -	select DRM_KMS_FB_HELPER
> -	select FB_CFB_FILLRECT
> -	select FB_CFB_COPYAREA
> -	select FB_CFB_IMAGEBLIT

Hm, your new generic fbdev emulation doesn't select the FB helpers we need
here, which probably breaks compilation. I think we just need to add all
these selects to the new core fbev emulation config - most drivers want
them anyway. And even for the drivers that have their own drawing routines
(udl and qxl afaik) the only thing they do is wrap calls to the ->dirty
callback around the cfb helpers. And we probably want to move this to the
fbdev helper library eventually anyway.

> -	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 the intel modesetting driver.
> -
> -	  If in doubt, say "Y".
> -
>  config DRM_I915_PRELIMINARY_HW_SUPPORT
>  	bool "Enable preliminary support for prerelease Intel hardware by default"
>  	depends on DRM_I915
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b7ddf48..355e43f 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -56,8 +56,8 @@ i915-y += intel_audio.o \
>  	  intel_psr.o \
>  	  intel_sideband.o \
>  	  intel_sprite.o
> -i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
> -i915-$(CONFIG_DRM_I915_FBDEV)	+= intel_fbdev.o
> +i915-$(CONFIG_ACPI)			+= intel_acpi.o intel_opregion.o
> +i915-$(CONFIG_DRM_FBDEV_EMULATION)	+= intel_fbdev.o
>  
>  # modesetting output/encoder code
>  i915-y += dvo_ch7017.o \
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 82bbe3f..bcce317 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1849,7 +1849,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
>  	struct intel_fbdev *ifbdev = NULL;
>  	struct intel_framebuffer *fb;
>  
> -#ifdef CONFIG_DRM_I915_FBDEV
> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	ifbdev = dev_priv->fbdev;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 542fac6..ab5881c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1798,11 +1798,9 @@ struct drm_i915_private {
>  
>  	struct drm_i915_gem_object *vlv_pctx;
>  
> -#ifdef CONFIG_DRM_I915_FBDEV

Hm for i915 I'd prefer to just replace one Kconfig symbol with the new one
and leave all the #ifdefs around. We try hard to remove all the fbdev
related code completely and that won't be the case when reintroducing some
of the runtime checks like you do. We could do something like

#ifdef CONFIG_DRM_FBDEV_EMULATION
#define HAS_FBDEV(dev_priv) 0
#else
#define HAS_FBDEV(dev_priv) ((dev_priv)->fbdev)
#endif

though. But really doing that is not your job ;-)

Cheers, Daniel

>  	/* list of fbdev register on this device */
>  	struct intel_fbdev *fbdev;
>  	struct work_struct fbdev_suspend_work;
> -#endif
>  
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1b61f98..0acd801 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9886,7 +9886,6 @@ static struct drm_framebuffer *
>  mode_fits_in_fbdev(struct drm_device *dev,
>  		   struct drm_display_mode *mode)
>  {
> -#ifdef CONFIG_DRM_I915_FBDEV
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_framebuffer *fb;
> @@ -9909,9 +9908,6 @@ mode_fits_in_fbdev(struct drm_device *dev,
>  		return NULL;
>  
>  	return fb;
> -#else
> -	return NULL;
> -#endif
>  }
>  
>  static int intel_modeset_setup_plane_state(struct drm_atomic_state *state,
> @@ -14296,11 +14292,13 @@ intel_user_framebuffer_create(struct drm_device *dev,
>  	return intel_framebuffer_create(dev, mode_cmd, obj);
>  }
>  
> -#ifndef CONFIG_DRM_I915_FBDEV
>  static inline void intel_fbdev_output_poll_changed(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->fbdev)
> +		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>  }
> -#endif
>  
>  static const struct drm_mode_config_funcs intel_mode_funcs = {
>  	.fb_create = intel_user_framebuffer_create,
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 6e4cc53..66eaf6f 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -395,18 +395,20 @@ static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
>  
>  static void intel_connector_add_to_fbdev(struct intel_connector *connector)
>  {
> -#ifdef CONFIG_DRM_I915_FBDEV
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> -	drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper, &connector->base);
> -#endif
> +
> +	if (dev_priv->fbdev)
> +		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
> +			&connector->base);
>  }
>  
>  static void intel_connector_remove_from_fbdev(struct intel_connector *connector)
>  {
> -#ifdef CONFIG_DRM_I915_FBDEV
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> -	drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper, &connector->base);
> -#endif
> +
> +	if (dev_priv->fbdev)
> +		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
> +			&connector->base);
>  }
>  
>  static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, const char *pathprop)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1059283..371b201 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1207,12 +1207,11 @@ void intel_dvo_init(struct drm_device *dev);
>  
>  
>  /* legacy fbdev emulation in intel_fbdev.c */
> -#ifdef CONFIG_DRM_I915_FBDEV
> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>  extern int intel_fbdev_init(struct drm_device *dev);
>  extern void intel_fbdev_initial_config(void *data, async_cookie_t cookie);
>  extern void intel_fbdev_fini(struct drm_device *dev);
>  extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous);
> -extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
>  extern void intel_fbdev_restore_mode(struct drm_device *dev);
>  #else
>  static inline int intel_fbdev_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 4aa21f7..445c35f 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -798,13 +798,6 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
>  	console_unlock();
>  }
>  
> -void intel_fbdev_output_poll_changed(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	if (dev_priv->fbdev)
> -		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> -}
> -
>  void intel_fbdev_restore_mode(struct drm_device *dev)
>  {
>  	int ret;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch