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

[Archit Taneja <architt@codeaurora.org>]

On 03/10/2015 03:31 PM, Daniel Vetter wrote:
> On Tue, Mar 10, 2015 at 03:11:30PM +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 #ifdefs
>> in certain places.
>>
>> 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
>> -	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 f019225..3b3325d 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 e8b18e5..0c8bcd7 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1772,7 +1772,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 8727086..e511fa4 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1819,11 +1819,9 @@ struct drm_i915_private {
>>
>>   	struct drm_i915_gem_object *vlv_pctx;
>>
>> -#ifdef CONFIG_DRM_I915_FBDEV
>>   	/* 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 e730789..9cf13e6 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -8562,7 +8562,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;
>> @@ -8585,9 +8584,6 @@ mode_fits_in_fbdev(struct drm_device *dev,
>>   		return NULL;
>>
>>   	return fb;
>> -#else
>> -	return NULL;
>> -#endif
>
> Won't this now blow up if there's no fbdev? Tbh I prefer a patch to just
> replace the CONFIG_ #define and then maybe a 2nd one on top to clean
> things up if you feel like. Same for the other changes in this patch.

There are checks for dev_priv->fbdev and dev_priv->fbdev->fb being non 
NULL at the start of the function which would prevent that. But you're 
right, the first patch for each device can replace the config, the later 
ones can take advantage of the stub functions.

Archit

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