| Message ID | 20181018233447.5187-5-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | re-organize a bit gen10 and gen11 | expand |
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(). 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. ;) 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",
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 :-) > 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
Quoting Daniel Vetter (2018-10-19 11:30:46) > 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 :-) > > > 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. Like Jani mentioned. I'm all in favour of going with a number of has_* flags, when there is mix and match between different gens. Regards, Joonas > -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
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. > > > 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
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
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(). > > 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. I think we should have indeed a display_gen in device info. It's a way to group features that are particular to that gen. Just like we do for gem. When the spec is clear about this being something from that gen, I think it's fine. We will need more fine grained flags in the device info, but IMO that should be in addition to this display gen number. Lucas De Marchi > > 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. ;) > > > 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
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. I think we should approximate to the closest one, but prefer using gen_display number checks for cases in which it's clear from the spec the separation. For other things we will need more fine grained flags, that can be used across gens or to differentiate features within a single gen. Lucas De Marchi > > > > > > 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
On Fri, Oct 19, 2018 at 09:41:37AM -0700, Rodrigo Vivi wrote: > 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... we could. Assuming we order the enum suitably. Problem is that there may not be a single order that works for all cases. But it might be good enough for a lot of the cases.
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",
Continuing with the goal of use less platform codenames: let's group platforms who has gen10 display. 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(-)