[Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Noralf Trønnes]

Den 04.02.2019 16.41, skrev Daniel Vetter:
> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
>> The only thing now that makes drm_dev_unplug() special is that it sets
>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
>> can remove drm_dev_unplug().
>>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---

[...]

>>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
>>  include/drm/drm_drv.h     | 10 ++++------
>>  2 files changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 05bbc2b622fc..e0941200edc6 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>>   */
>>  void drm_dev_unplug(struct drm_device *dev)
>>  {
>> -	/*
>> -	 * After synchronizing any critical read section is guaranteed to see
>> -	 * the new value of ->unplugged, and any critical section which might
>> -	 * still have seen the old value of ->unplugged is guaranteed to have
>> -	 * finished.
>> -	 */
>> -	dev->unplugged = true;
>> -	synchronize_srcu(&drm_unplug_srcu);
>> -
>>  	drm_dev_unregister(dev);
>>  	drm_dev_put(dev);
>>  }
>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>>   * drm_dev_register() but does not deallocate the device. The caller must call
>>   * drm_dev_put() to drop their final reference.
>>   *
>> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
>> - * which can be called while there are still open users of @dev.
>> + * This function can be called while there are still open users of @dev as long
>> + * as the driver protects its device resources using drm_dev_enter() and
>> + * drm_dev_exit().
>>   *
>>   * This should be called first in the device teardown code to make sure
>> - * userspace can't access the device instance any more.
>> + * userspace can't access the device instance any more. Drivers that support
>> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> 
> Read once more with a bit more coffee, spotted this:
> 
> s/first/afterwards/ - shutting down the hw before we've taken it away from
> userspace is kinda the wrong way round. It should be the inverse of driver
> load, which is 1) allocate structures 2) prep hw 3) register driver with
> the world (simplified ofc).
> 

The problem is that drm_dev_unregister() sets the device as unplugged
and if drm_atomic_helper_shutdown() is called afterwards it's not
allowed to touch hardware.

I know it's the wrong order, but the only way to do it in the right
order is to have a separate function that sets unplugged:

	drm_dev_unregister();
	drm_atomic_helper_shutdown();
	drm_dev_set_unplugged();

Noralf.

>> + * in order to disable the hardware on regular driver module unload.
>>   */
>>  void drm_dev_unregister(struct drm_device *dev)
>>  {
>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>>  		drm_lastclose(dev);
>>  
>> +	/*
>> +	 * After synchronizing any critical read section is guaranteed to see
>> +	 * the new value of ->unplugged, and any critical section which might
>> +	 * still have seen the old value of ->unplugged is guaranteed to have
>> +	 * finished.
>> +	 */
>> +	dev->unplugged = true;
>> +	synchronize_srcu(&drm_unplug_srcu);
>> +
>>  	dev->registered = false;
>>  
>>  	drm_client_dev_unregister(dev);
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index ca46a45a9cce..c50696c82a42 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>>   * drm_dev_is_unplugged - is a DRM device unplugged
>>   * @dev: DRM device
>>   *
>> - * This function can be called to check whether a hotpluggable is unplugged.
>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
>> - * unplugged, these two functions guarantee that any store before calling
>> - * drm_dev_unplug() is visible to callers of this function after it completes
>> + * This function can be called to check whether @dev is unregistered. This can
>> + * be used to detect that the underlying parent device is gone.
> 
> I think it'd be good to keep the first part, and just update the reference
> to drm_dev_unregister. So:
> 
>  * This function can be called to check whether a hotpluggable is unplugged.
>  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
>  * unplugged, these two functions guarantee that any store before calling
>  * drm_dev_unregister() is visible to callers of this function after it
>  * completes.
> 
> I think your version shrugs a few important details under the rug. With
> those nits addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> Cheers, Daniel
> 
>>   *
>> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
>> - * recommended that drivers instead use the underlying drm_dev_enter() and
>> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
>> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>>   * drm_dev_exit() function pairs.
>>   */
>>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>