Re: [PATCH 03/23] drm/i915: Move HAS_RUNTIME_PM definition to platform

[Carlos Santa <carlos.santa@intel.com>]

On Tue, 2016-08-09 at 16:49 +0300, Ville Syrjälä wrote:
> On Thu, Jul 21, 2016 at 04:34:28PM +0300, Imre Deak wrote:
> > On ke, 2016-07-20 at 13:25 -0700, Rodrigo Vivi wrote:
> > > On Wed, Jul 20, 2016 at 10:40 AM, Carlos Santa  wrote:
> > > > Moving all GPU features to the platform struct definition allows for
> > > >         - standard place when adding new features from new platforms
> > > >         - possible to see supported features when dumping struct
> > > >           definitions
> > > > 
> > > > Signed-off-by: Carlos Santa <carlos.santa@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h | 6 ++----
> > > >  drivers/gpu/drm/i915/i915_pci.c | 7 ++++++-
> > > >  2 files changed, 8 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 6569eb7..7443b9a 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -769,6 +769,7 @@ struct intel_csr {
> > > >         func(is_preliminary) sep \
> > > >         func(has_fbc) sep \
> > > >         func(has_psr) sep \
> > > > +       func(has_runtime_pm) sep \
> > > >         func(has_pipe_cxsr) sep \
> > > >         func(has_hotplug) sep \
> > > >         func(cursor_needs_physical) sep \
> > > > @@ -2848,10 +2849,7 @@ struct drm_i915_cmd_table {
> > > >  #define HAS_DDI(dev)           (INTEL_INFO(dev)->has_ddi)
> > > >  #define HAS_FPGA_DBG_UNCLAIMED(dev)    (INTEL_INFO(dev)->has_fpga_dbg)
> > > >  #define HAS_PSR(dev)           (INTEL_INFO(dev)->has_psr)
> > > > -#define HAS_RUNTIME_PM(dev)    (IS_GEN6(dev) || IS_HASWELL(dev) || \
> > > > -                                IS_BROADWELL(dev) || IS_VALLEYVIEW(dev) || \
> > > > -                                IS_CHERRYVIEW(dev) || IS_SKYLAKE(dev) || \
> > > > -                                IS_KABYLAKE(dev) || IS_BROXTON(dev))
> > > 
> > > Why don't we have runtime_pm on Ivybridge since we have on
> > > sandybdrige? Imre, any idea?
> > 
> > I don't know what are the exact differences, in any case I haven't
> > tried to enable RPM on IVB. I think Ville had plans for this.
> 
> I tried it, it gave zero benefit with the downside of killing HPD. So
> I've changed my mind and now I want to see the SNB runtime PM support
> removed instead. So runtime PM would be only for HSW+/VLV/CHV.

V3 of this patch series already does this.

> 
> And we still need to fix HPD while runtime suspended. But at least
> it's theoretically possible for those platforms. I don't think we can
> do it for SNB/IVB and older machines. And polling is no answer as
> it just increases the power consumption. So adding ineffective
> runtime PM with HPD polling is just wasting more power than not
> runtime suspending in the first place.
> 
> > 
> > --Imre
> > 
> > > 
> > > > +#define HAS_RUNTIME_PM(dev)    (INTEL_INFO(dev)->has_runtime_pm)
> > > >  #define HAS_RC6(dev)           (INTEL_INFO(dev)->gen >= 6)
> > > >  #define HAS_RC6p(dev)          (IS_GEN6(dev) || IS_IVYBRIDGE(dev))
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > > > index 8b1311d..92ab3c2 100644
> > > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > > @@ -198,6 +198,7 @@ static const struct intel_device_info intel_ironlake_m_info = {
> > > >         .gen = 6, .num_pipes = 2, \
> > > >         .need_gfx_hws = 1, .has_hotplug = 1, \
> > > >         .has_fbc = 1, \
> > > > +       .has_runtime_pm = 1, \
> > > 
> > > This patch made me notice that we should define the
> > > 
> > > GEN7_FEATURE on GEN6_FEATURES + new changes as a followup of patch
> > > 02/32 or in that same patch.
> > > However for this case we should redefine .has_runtime_pm=0 on gen7,
> > > what is really strange.
> > > 
> > > Anyway, this patch itself has nothing wrong and just follows what it
> > > was set there already.
> > > any change related to my comments should be addressed in separated patches.
> > > So fell free to also use
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > >         .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
> > > >         .has_llc = 1, \
> > > >         GEN_DEFAULT_PIPEOFFSETS, \
> > > > @@ -241,6 +242,7 @@ static const struct intel_device_info intel_ivybridge_q_info = {
> > > >  #define VLV_FEATURES  \
> > > >         .gen = 7, .num_pipes = 2, \
> > > >         .has_psr = 1, \
> > > > +       .has_runtime_pm = 1, \
> > > >         .need_gfx_hws = 1, .has_hotplug = 1, \
> > > >         .ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
> > > >         .display_mmio_offset = VLV_DISPLAY_BASE, \
> > > > @@ -263,7 +265,8 @@ static const struct intel_device_info intel_valleyview_d_info = {
> > > >         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING, \
> > > >         .has_ddi = 1, \
> > > >         .has_fpga_dbg = 1, \
> > > > -       .has_psr = 1
> > > > +       .has_psr = 1, \
> > > > +       .has_runtime_pm = 1
> > > > 
> > > >  static const struct intel_device_info intel_haswell_d_info = {
> > > >         HSW_FEATURES,
> > > > @@ -312,6 +315,7 @@ static const struct intel_device_info intel_cherryview_info = {
> > > >         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> > > >         .is_cherryview = 1,
> > > >         .has_psr = 1,
> > > > +       .has_runtime_pm = 1,
> > > >         .display_mmio_offset = VLV_DISPLAY_BASE,
> > > >         GEN_CHV_PIPEOFFSETS,
> > > >         CURSOR_OFFSETS,
> > > > @@ -340,6 +344,7 @@ static const struct intel_device_info intel_broxton_info = {
> > > >         .has_ddi = 1,
> > > >         .has_fpga_dbg = 1,
> > > >         .has_fbc = 1,
> > > > +       .has_runtime_pm = 1,
> > > >         .has_pooled_eu = 0,
> > > >         GEN_DEFAULT_PIPEOFFSETS,
> > > >         IVB_CURSOR_OFFSETS,
> > > > --
> > > > 1.9.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > 
> > > 
> 

thanks,
Carlos Santa
Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx