Re: [Intel-gfx] [PATCH v5 6/7] drm/i915: Implement fbdev client callbacks

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 7514 bytes --]

Hi

Am 25.10.23 um 11:36 schrieb Hogander, Jouni:
> Hi Thomas, couple of inline commments/suggestions below.
> 
> On Wed, 2023-09-27 at 12:26 +0200, Thomas Zimmermann wrote:
>> Move code from ad-hoc fbdev callbacks into DRM client functions
>> and remove the old callbacks. The functions instruct the client
>> to poll for changed output or restore the display.
>>
>> The DRM core calls both, the old callbacks and the new client
>> helpers, from the same places. The new functions perform the same
>> operation as before, so there's no change in functionality.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   .../drm/i915/display/intel_display_driver.c   |  1 -
>>   drivers/gpu/drm/i915/display/intel_fbdev.c    | 11 ++++++++--
>>   drivers/gpu/drm/i915/display/intel_fbdev.h    |  9 --------
>>   drivers/gpu/drm/i915/i915_driver.c            | 22 -----------------
>> --
>>   4 files changed, 9 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> index 44b59ac301e69..ffdcddd1943e0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> @@ -96,7 +96,6 @@ void intel_display_driver_init_hw(struct
>> drm_i915_private *i915)
>>   static const struct drm_mode_config_funcs intel_mode_funcs = {
>>          .fb_create = intel_user_framebuffer_create,
>>          .get_format_info = intel_fb_get_format_info,
>> -       .output_poll_changed = intel_fbdev_output_poll_changed,
>>          .mode_valid = intel_mode_valid,
>>          .atomic_check = intel_atomic_check,
>>          .atomic_commit = intel_atomic_commit,
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index d9e69471a782a..39de61d4e7906 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -638,7 +638,7 @@ void intel_fbdev_set_suspend(struct drm_device
>> *dev, int state, bool synchronous
>>          intel_fbdev_hpd_set_suspend(dev_priv, state);
>>   }
>>   
>> -void intel_fbdev_output_poll_changed(struct drm_device *dev)
>> +static void intel_fbdev_output_poll_changed(struct drm_device *dev)
> 
> Now as this isn't drm_mode_config_funcs callback anymore: Maybe you
> could return error value/0 ?

Yes, of course. After i915 has been converted to use drm_client, we can 
turn all this into consistent error handling across all the various 
drivers' fbdev emulation.

> 
>>   {
>>          struct intel_fbdev *ifbdev = to_i915(dev)-
>>> display.fbdev.fbdev;
>>          bool send_hpd;
>> @@ -657,7 +657,7 @@ void intel_fbdev_output_poll_changed(struct
>> drm_device *dev)
>>                  drm_fb_helper_hotplug_event(&ifbdev->helper);
>>   }
>>   
>> -void intel_fbdev_restore_mode(struct drm_i915_private *dev_priv)
>> +static void intel_fbdev_restore_mode(struct drm_i915_private
> 
> Similar comment as above. I.e. return error value/0 ?

Same here.

Best regards
Thomas

> 
> BR,
> 
> Jouni Högander
> 
>> *dev_priv)
>>   {
>>          struct intel_fbdev *ifbdev = dev_priv->display.fbdev.fbdev;
>>   
>> @@ -681,11 +681,18 @@ static void
>> intel_fbdev_client_unregister(struct drm_client_dev *client)
>>   
>>   static int intel_fbdev_client_restore(struct drm_client_dev *client)
>>   {
>> +       struct drm_i915_private *dev_priv = to_i915(client->dev);
>> +
>> +       intel_fbdev_restore_mode(dev_priv);
>> +       vga_switcheroo_process_delayed_switch();
>> +
>>          return 0;
>>   }
>>   
>>   static int intel_fbdev_client_hotplug(struct drm_client_dev *client)
>>   {
>> +       intel_fbdev_output_poll_changed(client->dev);
>> +
>>          return 0;
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.h
>> b/drivers/gpu/drm/i915/display/intel_fbdev.h
>> index 04fd523a50232..8c953f102ba22 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.h
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.h
>> @@ -19,8 +19,6 @@ void intel_fbdev_initial_config_async(struct
>> drm_i915_private *dev_priv);
>>   void intel_fbdev_unregister(struct drm_i915_private *dev_priv);
>>   void intel_fbdev_fini(struct drm_i915_private *dev_priv);
>>   void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool
>> synchronous);
>> -void intel_fbdev_output_poll_changed(struct drm_device *dev);
>> -void intel_fbdev_restore_mode(struct drm_i915_private *dev_priv);
>>   struct intel_framebuffer *intel_fbdev_framebuffer(struct intel_fbdev
>> *fbdev);
>>   #else
>>   static inline int intel_fbdev_init(struct drm_device *dev)
>> @@ -44,13 +42,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_i915_private
>> *i915)
>> -{
>> -}
>>   static inline struct intel_framebuffer
>> *intel_fbdev_framebuffer(struct intel_fbdev *fbdev)
>>   {
>>          return NULL;
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c
>> b/drivers/gpu/drm/i915/i915_driver.c
>> index de19197d2e052..86460cd8167d1 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -924,27 +924,6 @@ static int i915_driver_open(struct drm_device
>> *dev, struct drm_file *file)
>>          return 0;
>>   }
>>   
>> -/**
>> - * i915_driver_lastclose - clean up after all DRM clients have
>> exited
>> - * @dev: DRM device
>> - *
>> - * Take care of cleaning up after all DRM clients have exited.  In
>> the
>> - * mode setting case, we want to restore the kernel's initial mode
>> (just
>> - * in case the last client left us in a bad state).
>> - *
>> - * Additionally, in the non-mode setting case, we'll tear down the
>> GTT
>> - * and DMA structures, since the kernel won't be using them, and
>> clea
>> - * up any GEM state.
>> - */
>> -static void i915_driver_lastclose(struct drm_device *dev)
>> -{
>> -       struct drm_i915_private *i915 = to_i915(dev);
>> -
>> -       intel_fbdev_restore_mode(i915);
>> -
>> -       vga_switcheroo_process_delayed_switch();
>> -}
>> -
>>   static void i915_driver_postclose(struct drm_device *dev, struct
>> drm_file *file)
>>   {
>>          struct drm_i915_file_private *file_priv = file->driver_priv;
>> @@ -1822,7 +1801,6 @@ static const struct drm_driver i915_drm_driver
>> = {
>>              DRIVER_SYNCOBJ_TIMELINE,
>>          .release = i915_driver_release,
>>          .open = i915_driver_open,
>> -       .lastclose = i915_driver_lastclose,
>>          .postclose = i915_driver_postclose,
>>          .show_fdinfo = PTR_IF(IS_ENABLED(CONFIG_PROC_FS),
>> i915_drm_client_fdinfo),
>>   
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]