Re: [RFC 4/8] drm/i915: Group gen 10 display.

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

On Fri, Oct 19, 2018 at 01:33:08PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 19, 2018 at 10:30:46AM +0200, Daniel Vetter wrote:
> > On Fri, Oct 19, 2018 at 11:03:53AM +0300, Jani Nikula wrote:
> > > On Thu, 18 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > > Continuing with the goal of use less platform codenames:
> > > > let's group platforms who has gen10 display.
> > > 
> > > Ahah, so this answers my question in the previous patch. ;)
> > > 
> > > So we already have HAS_GMCH_DISPLAY().
> > 
> > We also have HAS_DDI, which I guess you could call gen8 display :-)
> 
> There's also these interesting gen designations in some old docs:
> ctg/elk = gen5.5
> ilk = gen5.75
> I guess we could more or less call all of that as gen5 display.
> 
> And of course we have the other oddballs like vlv/chv which are
> sort of mismash of i965, ctg/elk, ibx, and custom stuff. Also
> pnv is mostly gen3 display except for a few bits that were
> snatched from i965.
> 
> Not sure we have enough numbers for all that without resotring to
> fractions. And no one could anyway remember what all the different
> numbers mean.

Thanks for all the comments. Yeap, the idea is not to use this series
as is but just start the discussion and evolve it.

Gen number by itself doesn't fit to display indeed, but neither
display-gen-number because we have the fraction and cases like glk
that is gen10-stripped-display+bxt :/

Also platform names are not enough by itself, like cannnonlake-with-port-f
because the sku lacks on having a different name. If you take a look to our
internal branch the same is about to happen with icl soon....

So my idea was that we first use feature {name, or number of something}
whenever possible. On the secondary case we use groups of things like
HAS_GMCH_DISPLAY, HAS_GEN10_DISPLAY, HAS_VLV_DISPLAY. And on last case
we use gen numbers.

My idea of preferring gen over platform names in general was that for
platform name we cannot use >= ICELAKE... :/

Well, maybe having at least the basic display number to use in cases
like this would solve this case, but I'm afraid it can get tricky
in some common areas and the use of IS_GENx or IS_DISPLAY_x could become
blur and mixed up.

I will play more with this series, removing the controversial parts
and see what I can do with boolean "has_"

but for now still using gen >= 11 instead of icelake while we don't
have a better suggestion that covers that...

Thanks,
Rodrigo.

> 
> > 
> > > If we're going to make gen10 display a thing, should we not generalize
> > > this to have an actual display gen in device info? Of course for most
> > > platforms it's going to be identical to the gem gen.
> > > 
> > > The question becomes, is display gen accurate enough? It's not enough to
> > > cover all of Geminilake I believe. Which makes me think, should we just
> > > add tons more device info flags to cover features at a detailed level? I
> > > think that's what Joonas advocates. Perhaps it bloats the device info,
> > > and causes increase in the number of device info blocks. But my gut
> > > feeling says together with truly immutable device info, there are
> > > compiler optimization benefits to be had.
> > > 
> > > Also, currently we "inherit" device info using truly obnoxious macro
> > > layering where you have to work hard to trace the macros to find out
> > > what is set for a given platform. It doesn't have to be that way. We
> > > could move parts of it to separate but shared features structs, and add
> > > pointers to them.
> > > 
> > > Anyway, thanks for rolling this series as a starting point for
> > > discussion. Even if I'm not buying the changes as-is. ;)
> > 
> > Yeah, stuffing this into intel_info imo makes sense. As long as it's
> > booleans they should be fairly cheap.
> > -Daniel
> > > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > >
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h      | 2 ++
> > > >  drivers/gpu/drm/i915/intel_cdclk.c   | 2 +-
> > > >  drivers/gpu/drm/i915/intel_color.c   | 2 +-
> > > >  drivers/gpu/drm/i915/intel_display.c | 4 ++--
> > > >  drivers/gpu/drm/i915/intel_sprite.c  | 2 +-
> > > >  5 files changed, 7 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index d19b38ef145b..6436fedfe561 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2637,6 +2637,8 @@ intel_info(const struct drm_i915_private *dev_priv)
> > > >  #define SUPPORTS_TV(dev_priv)		((dev_priv)->info.supports_tv)
> > > >  #define I915_HAS_HOTPLUG(dev_priv)	((dev_priv)->info.has_hotplug)
> > > >  
> > > > +#define HAS_DISPLAY_10(dev_priv) (IS_GEN10(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > > +
> > > >  #define HAS_FW_BLC(dev_priv) 	(INTEL_GEN(dev_priv) > 2)
> > > >  #define HAS_FBC(dev_priv)	((dev_priv)->info.has_fbc)
> > > >  #define HAS_CUR_FBC(dev_priv)	(!HAS_GMCH_DISPLAY(dev_priv) && INTEL_GEN(dev_priv) >= 7)
> > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > index 4aa6500604cc..b315b70fd49c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > @@ -2181,7 +2181,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> > > >  	    crtc_state->has_audio &&
> > > >  	    crtc_state->port_clock >= 540000 &&
> > > >  	    crtc_state->lane_count == 4) {
> > > > -		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> > > > +		if (HAS_DISPLAY_10(dev_priv)) {
> > > >  			/* Display WA #1145: glk,cnl */
> > > >  			min_cdclk = max(316800, min_cdclk);
> > > >  		} else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > > > index 5127da286a2b..d5f67b9c9698 100644
> > > > --- a/drivers/gpu/drm/i915/intel_color.c
> > > > +++ b/drivers/gpu/drm/i915/intel_color.c
> > > > @@ -659,7 +659,7 @@ void intel_color_init(struct drm_crtc *crtc)
> > > >  		   IS_BROXTON(dev_priv)) {
> > > >  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> > > >  		dev_priv->display.load_luts = broadwell_load_luts;
> > > > -	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> > > > +	} else if (HAS_DISPLAY_10(dev_priv)) {
> > > >  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> > > >  		dev_priv->display.load_luts = glk_load_luts;
> > > >  	} else {
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 7b04b8436b6d..1abf79a4ee91 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -5730,7 +5730,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> > > >  	intel_crtc->active = true;
> > > >  
> > > >  	/* Display WA #1180: WaDisableScalarClockGating: glk, cnl */
> > > > -	psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
> > > > +	psl_clkgate_wa = (HAS_DISPLAY_10(dev_priv)) &&
> > > >  			 pipe_config->pch_pfit.enabled;
> > > >  	if (psl_clkgate_wa)
> > > >  		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > > > @@ -15565,7 +15565,7 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
> > > >  static void intel_early_display_was(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	/* Display WA #1185 WaDisableDARBFClkGating:cnl,glk */
> > > > -	if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > > +	if (HAS_DISPLAY_10(dev_priv))
> > > >  		I915_WRITE(GEN9_CLKGATE_DIS_0, I915_READ(GEN9_CLKGATE_DIS_0) |
> > > >  			   DARBF_GATING_DIS);
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index 7cd59eee5cad..912ab7b9d89a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -1330,7 +1330,7 @@ static int skl_plane_check_dst_coordinates(const struct intel_crtc_state *crtc_s
> > > >  	 * than the cursor ending less than 4 pixels from the left edge of the
> > > >  	 * screen may cause FIFO underflow and display corruption.
> > > >  	 */
> > > > -	if ((IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
> > > > +	if (HAS_DISPLAY_10(dev_priv) &&
> > > >  	    (crtc_x + crtc_w < 4 || crtc_x > pipe_src_w - 4)) {
> > > >  		DRM_DEBUG_KMS("requested plane X %s position %d invalid (valid range %d-%d)\n",
> > > >  			      crtc_x + crtc_w < 4 ? "end" : "start",
> > > 
> > > -- 
> > > Jani Nikula, Intel Open Source Graphics Center
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx