Re: [PATCH v3 01/13] drm/xe: Fix xe_display_fini() calls

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 10-02-2025 23:26, Ghimiray, Himal Prasad wrote:
> 
> 
> On 08-02-2025 03:49, Lucas De Marchi wrote:
>> xe_display_fini() undoes things from xe_display_init() (technically from
>> intel_display_driver_probe()). Those `goto err` in xe_device_probe()
>> were wrong and being accumulated over time.
>>
>> Commit 65e366ace5ee ("drm/xe/display: Use a single early init call for
>> display") made it easier to fix now that we don't have xe_display_* init
>> calls spread on xe_device_probe(). Change xe_display_init() to use
>> devm_add_action_or_reset() that will finalize display in the right
>> order.
>>
>> While at it, also add a newline and comment about calling
>> xe_driver_flr_fini.
>>
>> Cc: Maarten Lankhorst <dev@lankhorst.se>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>   drivers/gpu/drm/xe/display/xe_display.c | 22 ++++++++++++----------
>>   drivers/gpu/drm/xe/display/xe_display.h |  2 --
>>   drivers/gpu/drm/xe/xe_device.c          | 21 +++++++++++----------
>>   3 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/ 
>> drm/xe/display/xe_display.c
>> index 651799c946ace..302f533af037d 100644
>> --- a/drivers/gpu/drm/xe/display/xe_display.c
>> +++ b/drivers/gpu/drm/xe/display/xe_display.c
>> @@ -161,27 +161,29 @@ int xe_display_init_early(struct xe_device *xe)
>>       return err;
>>   }
>> -int xe_display_init(struct xe_device *xe)
>> +static void xe_display_fini(void *arg)
>>   {
>> +    struct xe_device *xe = arg;
>>       struct intel_display *display = &xe->display;
>> -    if (!xe->info.probe_display)
>> -        return 0;
>> -
>> -    return intel_display_driver_probe(display);
>> +    intel_hpd_poll_fini(xe);
>> +    intel_hdcp_component_fini(display);
>> +    intel_audio_deinit(display);
>>   }
>> -void xe_display_fini(struct xe_device *xe)
>> +int xe_display_init(struct xe_device *xe)
>>   {
>>       struct intel_display *display = &xe->display;
>> +    int err;
>>       if (!xe->info.probe_display)
>> -        return;
>> +        return 0;
>> -    intel_hpd_poll_fini(xe);
>> +    err = intel_display_driver_probe(display);
>> +    if (err)
>> +        return err;
>> -    intel_hdcp_component_fini(display);
>> -    intel_audio_deinit(display);
>> +    return devm_add_action_or_reset(xe->drm.dev, xe_display_fini, xe);
>>   }
>>   void xe_display_register(struct xe_device *xe)
>> diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/ 
>> drm/xe/display/xe_display.h
>> index e2a99624f7064..685dc74402fb8 100644
>> --- a/drivers/gpu/drm/xe/display/xe_display.h
>> +++ b/drivers/gpu/drm/xe/display/xe_display.h
>> @@ -22,7 +22,6 @@ int xe_display_probe(struct xe_device *xe);
>>   int xe_display_init_early(struct xe_device *xe);
>>   int xe_display_init(struct xe_device *xe);
>> -void xe_display_fini(struct xe_device *xe);
>>   void xe_display_register(struct xe_device *xe);
>>   void xe_display_unregister(struct xe_device *xe);
>> @@ -54,7 +53,6 @@ static inline int xe_display_probe(struct xe_device 
>> *xe) { return 0; }
>>   static inline int xe_display_init_early(struct xe_device *xe) 
>> { return 0; }
>>   static inline int xe_display_init(struct xe_device *xe) { return 0; }
>> -static inline void xe_display_fini(struct xe_device *xe) {}
>>   static inline void xe_display_register(struct xe_device *xe) {}
>>   static inline void xe_display_unregister(struct xe_device *xe) {}
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/ 
>> xe_device.c
>> index 36d7ffb3b4d90..ac3e344fe04ec 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -735,6 +735,7 @@ static int probe_has_flat_ccs(struct xe_device *xe)
>>               "Flat CCS has been disabled in bios, May lead to 
>> performance impact");
>>       xe_force_wake_put(gt_to_fw(gt), fw_ref);
>> +
>>       return 0;
>>   }
>> @@ -803,22 +804,26 @@ int xe_device_probe(struct xe_device *xe)
>>       err = xe_devcoredump_init(xe);
>>       if (err)
>>           return err;
>> +
>> +    /*
>> +     * From here on, if a step fails, make sure a Driver-FLR is 
>> triggereed
>> +     */
>>       err = devm_add_action_or_reset(xe->drm.dev, xe_driver_flr_fini, 
>> xe);
>>       if (err)
>>           return err;
>>       err = probe_has_flat_ccs(xe);
>>       if (err)
>> -        goto err;
>> +        return err;
>>       err = xe_vram_probe(xe);
>>       if (err)
>> -        goto err;
>> +        return err;
>>       for_each_tile(tile, xe, id) {
>>           err = xe_tile_init_noalloc(tile);
>>           if (err)
>> -            goto err;
>> +            return err;
>>       }
>>       /* Allocate and map stolen after potential VRAM resize */
>> @@ -832,17 +837,17 @@ int xe_device_probe(struct xe_device *xe)
>>        */
>>       err = xe_display_init_early(xe);
>>       if (err)
>> -        goto err;
>> +        return err;
> 
> Currently, xe_oa_fini and xe_heci_gsc_fini won't be called in case of an 
> error. This is incomplete without also moving xe_oa_fini and 
> xe_heci_gsc_fini under devm_add_action_or_reset.

xe_oa_fini is addressed in further patches and xe_heci_gsc_fini is 
already in cleanup path. With that patch LGTM

Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

> 
> 
>>       for_each_tile(tile, xe, id) {
>>           err = xe_tile_init(tile);
>>           if (err)
>> -            goto err;
>> +            return err;
>>       }
>>       err = xe_irq_install(xe);
>>       if (err)
>> -        goto err;
>> +        return err;
>>       for_each_gt(gt, xe, id) {
>>           last_gt = id;
>> @@ -902,8 +907,6 @@ int xe_device_probe(struct xe_device *xe)
>>               break;
>>       }
>> -err:
>> -    xe_display_fini(xe);
>>       return err;
>>   }
>> @@ -924,8 +927,6 @@ void xe_device_remove(struct xe_device *xe)
>>       xe_device_remove_display(xe);
>> -    xe_display_fini(xe);
>> -
>>       xe_oa_fini(xe);
>>       xe_heci_gsc_fini(xe);
>