[Intel-gfx] [PATCH v2 08/15] drm/i915: Use drm_fb_helper_output_poll_changed()

[Noralf Trønnes]

Den 31.10.2017 11.27, skrev Daniel Vetter:
> On Mon, Oct 30, 2017 at 04:39:44PM +0100, Noralf Trønnes wrote:
>> This driver can use drm_fb_helper_output_poll_changed() as its
>> .output_poll_changed callback.
>>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 2 +-
>>   drivers/gpu/drm/i915/intel_drv.h     | 5 -----
>>   drivers/gpu/drm/i915/intel_fbdev.c   | 8 --------
>>   3 files changed, 1 insertion(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f780f39e0758..b205e2c782bb 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14123,7 +14123,7 @@ static void intel_atomic_state_free(struct drm_atomic_state *state)
>>   static const struct drm_mode_config_funcs intel_mode_funcs = {
>>   	.fb_create = intel_user_framebuffer_create,
>>   	.get_format_info = intel_get_format_info,
>> -	.output_poll_changed = intel_fbdev_output_poll_changed,
>> +	.output_poll_changed = drm_fb_helper_output_poll_changed,
>>   	.atomic_check = intel_atomic_check,
>>   	.atomic_commit = intel_atomic_commit,
>>   	.atomic_state_alloc = intel_atomic_state_alloc,
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 463ed152e6b1..dfcf5ba220e8 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1607,7 +1607,6 @@ extern void intel_fbdev_initial_config_async(struct drm_device *dev);
>>   extern void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
>>   extern void intel_fbdev_fini(struct drm_i915_private *dev_priv);
>>   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)
>> @@ -1631,10 +1630,6 @@ static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bo
>>   {
>>   }
>>   
>> -static inline void intel_fbdev_output_poll_changed(struct drm_device *dev)
>> -{
>> -}
>> -
>>   static inline void intel_fbdev_restore_mode(struct drm_device *dev)
>>   {
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>> index f2bb8116227c..35babbadfc5a 100644
>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>> @@ -796,14 +796,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 intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>> -
>> -	if (ifbdev)
> This removes the NULL check here, and I think we still need that one for
> some slightly unclear but probably hilarious reasons.
>
> I guess simplest if you just drop the i915 patch as too tricky.

Ok.

Several drivers had this NULL check and I did a verification in the drivers
fbdev code that conversion was OK. But I left it to the maintainer to know
about any code paths that wasn't obvious to me by looking at the fbdev code.

On the surface i915 looks OK and here's the rationale:

dev->fb_helper is set in drm_fb_helper_init() and cleared in 
drm_fb_helper_fini().
All fbdev init error paths I've seen call drm_fb_helper_fini().
This means that dev->fb_helper is only set when fbdev is initialized.

drm_fb_helper_output_poll_changed() can handle dev->fb_helper == NULL:

void drm_fb_helper_output_poll_changed(struct drm_device *dev)
{
     drm_fb_helper_hotplug_event(dev->fb_helper);
}

int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
{
...
     if (!drm_fbdev_emulation || !fb_helper)
          return 0;
...
}

dev->fb_helper and dev_priv->fbdev are set and cleared in sync:

int intel_fbdev_init(struct drm_device *dev)
{
     struct drm_i915_private *dev_priv = to_i915(dev);
     struct intel_fbdev *ifbdev;
     int ret;

     if (WARN_ON(INTEL_INFO(dev_priv)->num_pipes == 0))
         return -ENODEV;

     ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
     if (ifbdev == NULL)
         return -ENOMEM;

     drm_fb_helper_prepare(dev, &ifbdev->helper, &intel_fb_helper_funcs);

     if (!intel_fbdev_init_bios(dev, ifbdev))
         ifbdev->preferred_bpp = 32;

     ret = drm_fb_helper_init(dev, &ifbdev->helper, 4);
     if (ret) {
         kfree(ifbdev);
         return ret;
     }

dev->fb_helper is now set (by drm_fb_helper_init()).

     dev_priv->fbdev = ifbdev;

dev_priv->fbdev is now set

     INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);

     drm_fb_helper_single_add_all_connectors(&ifbdev->helper);

     return 0;
}

void intel_fbdev_fini(struct drm_i915_private *dev_priv)
{
     struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv->fbdev);

dev_priv->fbdev is now NULL

     if (!ifbdev)
         return;

     intel_fbdev_destroy(ifbdev);
}

static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
{
     /* We rely on the object-free to release the VMA pinning for
      * the info->screen_base mmaping. Leaking the VMA is simpler than
      * trying to rectify all the possible error paths leading here.
      */

     drm_fb_helper_fini(&ifbdev->helper);

dev->fb_helper is now NULL (cleared by drm_fb_helper_fini())

     if (ifbdev->vma) {
         mutex_lock(&ifbdev->helper.dev->struct_mutex);
         intel_unpin_fb_vma(ifbdev->vma);
         mutex_unlock(&ifbdev->helper.dev->struct_mutex);
     }

     if (ifbdev->fb)
         drm_framebuffer_remove(&ifbdev->fb->base);

     kfree(ifbdev);
}


But I understand that it's better to be safe than sorry :-)

Hopefully all your CI work will make checking patches like this a breeze 
one day.

Noralf.

> -Daniel
>
>> -		drm_fb_helper_hotplug_event(&ifbdev->helper);
>> -}
>> -
>>   void intel_fbdev_restore_mode(struct drm_device *dev)
>>   {
>>   	struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>> -- 
>> 2.14.2
>>