Re: [RFC v2 0/4] drm/i915: better high level abstraction for display

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Mar 06, 2024 at 02:24:34PM +0200, Jani Nikula wrote:
> This is v2 of [1]. Improve the abstractions for display code.
> 
> The main goals are:
> 
> 1) The display code does not access struct drm_i915_private or struct
>    xe_device. It only uses its own struct intel_display instead.
> 
> 2) The i915 and xe driver cores do not access struct intel_display
>    directly. It becomes an opaque pointer to them, stored in struct
>    drm_i915_private and struct xe_device, and passed to display code.
> 
> This will mean a lot of churn, unfortunately. But it will better
> separate the display code from the xe and i915 driver cores, and pave
> the way for a) removing -Ddrm_i915_private=xe_device from xe Makefile,
> and b) stop building the display code twice.
> 
> What's presented here goes a long way, and could get us started. 

I believe this series is a great start towards the separation goal
and I liked the approach.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
for the series.

> But
> there are still opens, such as:
> 
> 1) How to handle platform checks such as IS_TIGERLAKE().

perhaps we can use display_ver and make up random release version
table.
display.verion = 12
display.release = / 0->TIGERLAKE 1->ALDERLAKE, etc...

or just some specific display macros IS_TIGERLAKE_DISPLAY(display)

> 
> 2) How to handle access to non-display members of i915/xe, such as
>    i915->uncore.

Perhaps some driver registration callbacks with the mmio functions?

the worst part here will be
drivers/gpu/drm/i915/display/intel_vblank.c:    __acquires(i915->uncore.lock)
drivers/gpu/drm/i915/display/intel_vblank.c:    spin_lock(&i915->uncore.lock);

and for that perhaps we need some different lock? or a lock pointer along
with the mmio calback on a registration struct?

> 
> There are other similar things, but I believe those are the most
> prevalent, and are the biggest blockers for converting a lot of
> functions over from i915 -> intel_display.
> 
> 
> BR,
> Jani.
> 
> [1] https://lore.kernel.org/r/cover.1695747484.git.jani.nikula@intel.com
> 
> 
> Jani Nikula (4):
>   drm/i915/display: ideas for further separating display code from the
>     rest
>   drm/i915/display: add generic to_intel_display() macro
>   drm/i915/display: accept either i915 or display for feature tests
>   drm/i915/display: test various to_intel_display() scenarios
> 
>  .../gpu/drm/i915/display/intel_display_core.h |  3 ++
>  .../drm/i915/display/intel_display_device.c   | 13 ++++++
>  .../drm/i915/display/intel_display_device.h   | 10 +++-
>  .../drm/i915/display/intel_display_types.h    | 46 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c       |  6 +--
>  drivers/gpu/drm/i915/display/intel_hdmi.c     | 13 +++---
>  drivers/gpu/drm/i915/i915_drv.h               | 11 ++++-
>  drivers/gpu/drm/xe/xe_device_types.h          | 15 +++++-
>  8 files changed, 103 insertions(+), 14 deletions(-)
> 
> -- 
> 2.39.2
>