[PATCH v2 1/3] drm: Add callbacks for late registering

[Benjamin Gaignard]

2016-06-21 12:31 GMT+02:00 Chris Wilson <chris at chris-wilson.co.uk>:
> On Tue, Jun 21, 2016 at 11:31:34AM +0200, Benjamin Gaignard wrote:
>> Like what has been done for connectors add callbacks on encoder,
>> crtc and plane to let driver do actions after drm device registration.
>>
>> Correspondingly, add callbacks called before unregister drm device.
>>
>> version 2:
>> add drm_modeset_register_all() and drm_modeset_unregister_all()
>> to centralize all calls
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard at linaro.org>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 122 +++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_drv.c  |   4 +-
>>  include/drm/drm_crtc.h     |  79 +++++++++++++++++++++++++++++
>>  3 files changed, 203 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index e7c862b..b393feb 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -608,6 +608,31 @@ static unsigned int drm_num_crtcs(struct drm_device *dev)
>>       return num;
>>  }
>>
>> +static int drm_crtc_register_all(struct drm_device *dev)
>> +{
>> +     struct drm_crtc *crtc;
>> +     int ret;
>> +
>> +     drm_for_each_crtc(crtc, dev) {
>> +             if (crtc->funcs->late_register)
>> +                     ret = crtc->funcs->late_register(crtc);
>> +             if (ret)
>
> ret is uninitialised.

OK I fix that for v3

>
> It is a good question (probably for another series) on what exactly to
> do on registration failure? At the very least we need to unwind on the
> error path, or we just ignore errors (other than a DRM_ERROR to
> userspace explaining why one object is not available, but otherwise let
> the driver load).
>
>> +int drm_modeset_register_all(struct drm_device *dev)
>> +{
>> +     int ret;
>> +
>> +     ret = drm_plane_register_all(dev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_crtc_register_all(dev);
>> +     if  (ret)
>> +             return ret;
>> +
>> +     ret = drm_encoder_register_all(dev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_connector_register_all(dev);
>
> Might as well continue on with
>
> ret = <>
> if (ret)
>         return ret;
>
> for a consistent style. Along with the comment about how should we be
> handling errors? If we report an error, everything up to that point
> should be unwound.
>

OK I will add goto for each case.

>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(drm_modeset_register_all);
>> +
>> +/**
>> + * drm_modeset_unregister_all - do early unregistration
>> + * @dev: drm device
>> + *
>> + * This function call early_unregister callbakc for all planes,
>> + * crtcs, encoders and connectors
>> + */
>> +void drm_modeset_unregister_all(struct drm_device *dev)
>> +{
>> +     drm_plane_unregister_all(dev);
>> +     drm_crtc_unregister_all(dev);
>> +     drm_encoder_unregister_all(dev);
>> +     drm_connector_unregister_all(dev);
>
> Unregister should be in the opposite order.

OK for v3

>> +}
>> +EXPORT_SYMBOL(drm_modeset_unregister_all);
>
> I think the plan is not to export these symbols,
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Benjamin Gaignard

Graphic Working Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog