[Intel-gfx] [PATCH 01/39] drm/i915: add display sub-struct to drm_i915_private

[Murthy, Arun R]

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Tuesday, August 16, 2022 1:07 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: De Marchi, Lucas <lucas.demarchi at intel.com>
> Subject: RE: [Intel-gfx] [PATCH 01/39] drm/i915: add display sub-struct to
> drm_i915_private
> 
> On Tue, 16 Aug 2022, "Murthy, Arun R" <arun.r.murthy at intel.com> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani <jani.nikula at intel.com>
> >> Sent: Friday, August 12, 2022 12:10 PM
> >> To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-
> >> gfx at lists.freedesktop.org
> >> Cc: De Marchi, Lucas <lucas.demarchi at intel.com>
> >> Subject: RE: [Intel-gfx] [PATCH 01/39] drm/i915: add display
> >> sub-struct to drm_i915_private
> >>
> >> On Fri, 12 Aug 2022, "Murthy, Arun R" <arun.r.murthy at intel.com> wrote:
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> >> >> @@ -0,0 +1,38 @@
> >> >> +/* SPDX-License-Identifier: MIT */
> >> >> +/*
> >> >> + * Copyright © 2022 Intel Corporation  */
> >> >> +
> >> >> +#ifndef __INTEL_DISPLAY_CORE_H__
> >> >> +#define __INTEL_DISPLAY_CORE_H__
> >> >> +
> >> >> +#include <linux/types.h>
> >> >> +
> >> >> +struct intel_atomic_state;
> >> >> +struct intel_crtc;
> >> >> +struct intel_crtc_state;
> >> >> +struct intel_initial_plane_config;
> >> >> +
> >> >> +struct intel_display_funcs {
> >> >> +     /* Returns the active state of the crtc, and if the crtc is active,
> >> >> +      * fills out the pipe-config with the hw state. */
> >> > Can this be changed to multi-line commenting style.
> >>
> >> Yeah.
> >>
> >> > /*
> >> >  *
> >> >  */
> >> >> +     bool (*get_pipe_config)(struct intel_crtc *,
> >> >> +                             struct intel_crtc_state *);
> >> >> +     void (*get_initial_plane_config)(struct intel_crtc *,
> >> >> +                                      struct intel_initial_plane_config *);
> >> >> +     void (*crtc_enable)(struct intel_atomic_state *state,
> >> >> +                         struct intel_crtc *crtc);
> >> >> +     void (*crtc_disable)(struct intel_atomic_state *state,
> >> >> +                          struct intel_crtc *crtc);
> >> >> +     void (*commit_modeset_enables)(struct intel_atomic_state
> >> >> + *state);
> >> >
> >> > Can this be changed to something meaningful word, something like
> >> > update_modeset()
> >>
> >> It's already borderline doing too much in one patch to rename the
> >> struct, and definitely too much to rename the hook. Maybe in another
> patch.
> >>
> > Thanks for accepting, would this come as part of this series itself?
> 
> I think this series is already growing too big, I'll want to start merging before
> adding new patches. But there's more things to move to the display sub-
> struct too, so there'll be more patches.
> 
I hope a TODO would be added for this!

Reviewed-by: Arun R Murthy <arun.r.murthy at intel.com>

Thanks and Regards,
Arun R Murthy
--------------------