[PATCH 5/9] drm/xe/display: Drop xe_display_driver_remove()

[Cavitt, Jonathan]

-----Original Message-----
From: De Marchi, Lucas <lucas.demarchi at intel.com> 
Sent: Friday, February 21, 2025 2:49 PM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
Cc: intel-xe at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at intel.com>; Nikula, Jani <jani.nikula at intel.com>
Subject: Re: [PATCH 5/9] drm/xe/display: Drop xe_display_driver_remove()
> 
> On Fri, Feb 14, 2025 at 10:19:10PM +0000, Cavitt, Jonathan wrote:
> >-----Original Message-----
> >From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Lucas De Marchi
> >Sent: Friday, February 14, 2025 1:23 PM
> >To: intel-xe at lists.freedesktop.org
> >Cc: De Marchi, Lucas <lucas.demarchi at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>; Nikula, Jani <jani.nikula at intel.com>
> >Subject: [PATCH 5/9] drm/xe/display: Drop xe_display_driver_remove()
> >>
> >> Handle it as part of xe_display_fini(). The error handling was already
> >> calling it if a step after xe_display_init() failed. Just re-use the
> >> same xe_display_fini() for driver remove.
> >>
> >> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >> Cc: Jani Nikula <jani.nikula at intel.com>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> >
> >Looks good to me, though we can probably remove the first
> >sentence of the commit message as it makes it sound like
> >we're adding the functionality into xe_display_fini, rather than
> >leveraging functionality that's already there.
> 
> humn... I'm confused. See below
> 
> >Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> >-Jonathan Cavitt
> >
> >> ---
> >>  drivers/gpu/drm/xe/display/xe_display.c | 11 +----------
> >>  drivers/gpu/drm/xe/display/xe_display.h |  1 -
> >>  drivers/gpu/drm/xe/xe_device.c          |  8 ++------
> >>  3 files changed, 3 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> >> index 5ad2c99a9ae74..b7c99b8df11f9 100644
> >> --- a/drivers/gpu/drm/xe/display/xe_display.c
> >> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> >> @@ -170,6 +170,7 @@ static void xe_display_fini(void *arg)
> >>  	intel_hpd_poll_fini(xe);
> >>  	intel_hdcp_component_fini(display);
> >>  	intel_audio_deinit(display);
> >> +	intel_display_driver_remove(display);
> 
> ^ here
> 
> what I meant is that we don't have to do this manually from
> xe_device_remove(). We can just do it from xe_display_fini()
> that is already called by the cleanup functions.
> 
> Do you think a reword would be better?

Erhm... No, I think I just misread the patch when I did the initial review and
somehow missed you adding intel_display_driver_remove() to xe_display_fini(),
probably thinking it was already there?  It's been a week, so I don't know for certain.

My Reviewed-by still stands.
-Jonathan Cavitt

> 
> thanks
> Lucas De Marchi
>