[PATCH v2 10/10] drm/fb-helper: Remove return value from drm_fbdev_generic_setup()

[Thomas Zimmermann]

Hi Sam

Am 08.04.20 um 10:50 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> You missed my ack on the first 9 patches:
> https://lore.kernel.org/dri-devel/20200407101354.GA12686@ravnborg.org/
> Not that it matters as others have acked/reviewed them.

This got lost. I'll add you acks. Thanks for taking another look at the
patches.

> 
> On Wed, Apr 08, 2020 at 10:26:41AM +0200, Thomas Zimmermann wrote:
>> Generic fbdev emulation is a DRM client. Drivers should invoke the
>> setup function, but not depend on its success. Hence remove the return
>> value.
>>
>> v2:
>> 	* warn if fbdev device has not been registered yet
>> 	* document the new behavior
>> 	* convert the existing warning to the new dev_ interface
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Reviewed-by: Sam Ravnborg <sam at ravnborg.org>
>> Reviewed-by: Noralf Trønnes <noralf at tronnes.org>
>> Acked-by: Gerd Hoffmann <kraxel at redhat.com>
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++------------
>>  include/drm/drm_fb_helper.h     |  5 +++--
>>  2 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 165c8dab50797..97f5e43837486 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2169,7 +2169,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>>   *                 @dev->mode_config.preferred_depth is used if this is zero.
>>   *
>>   * This function sets up generic fbdev emulation for drivers that supports
>> - * dumb buffers with a virtual address and that can be mmap'ed.
>> + * dumb buffers with a virtual address and that can be mmap'ed. It's supposed
>> + * to run after the DRM driver registered the new DRM device with
>> + * drm_dev_register().
> OR maybe be more explicit - something like:
> drm_fbdev_generic_setup() shall be called after the DRM is registered
> with drm_dev_register().

I think this one is even better.

Best regards
Thomas

> 
> Either way is fine.
> 
> 	Sam
> 
>>   *
>>   * Restore, hotplug events and teardown are all taken care of. Drivers that do
>>   * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
>> @@ -2186,29 +2188,30 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>>   * Setup will be retried on the next hotplug event.
>>   *
>>   * The fbdev is destroyed by drm_dev_unregister().
>> - *
>> - * Returns:
>> - * Zero on success or negative error code on failure.
>>   */
>> -int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>> +void drm_fbdev_generic_setup(struct drm_device *dev,
>> +			     unsigned int preferred_bpp)
>>  {
>>  	struct drm_fb_helper *fb_helper;
>>  	int ret;
>>  
>> -	WARN(dev->fb_helper, "fb_helper is already set!\n");
>> +	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
>> +	drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>>  
>>  	if (!drm_fbdev_emulation)
>> -		return 0;
>> +		return;
>>  
>>  	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
>> -	if (!fb_helper)
>> -		return -ENOMEM;
>> +	if (!fb_helper) {
>> +		drm_err(dev, "Failed to allocate fb_helper\n");
>> +		return;
>> +	}
>>  
>>  	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
>>  	if (ret) {
>>  		kfree(fb_helper);
>>  		drm_err(dev, "Failed to register client: %d\n", ret);
>> -		return ret;
>> +		return;
>>  	}
>>  
>>  	if (!preferred_bpp)
>> @@ -2222,8 +2225,6 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>>  		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>>  
>>  	drm_client_register(&fb_helper->client);
>> -
>> -	return 0;
>>  }
>>  EXPORT_SYMBOL(drm_fbdev_generic_setup);
>>  
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 208dbf87afa3e..fb037be83997d 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -269,7 +269,8 @@ int drm_fb_helper_debug_leave(struct fb_info *info);
>>  void drm_fb_helper_lastclose(struct drm_device *dev);
>>  void drm_fb_helper_output_poll_changed(struct drm_device *dev);
>>  
>> -int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp);
>> +void drm_fbdev_generic_setup(struct drm_device *dev,
>> +			     unsigned int preferred_bpp);
>>  #else
>>  static inline void drm_fb_helper_prepare(struct drm_device *dev,
>>  					struct drm_fb_helper *helper,
>> @@ -443,7 +444,7 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
>>  {
>>  }
>>  
>> -static inline int
>> +static inline void
>>  drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>>  {
>>  	return 0;
>> -- 
>> 2.26.0

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200408/10260eb8/attachment.sig>