diff mbox

[4/4] drm/i915/fbc: enable FBC on SKL too

Message ID 1458588418-31129-6-git-send-email-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R March 21, 2016, 7:26 p.m. UTC
Now that we're more protected against user space doing frontbuffer
mmap rendering, the last - how many times did I say this before? -
SKL problem seems to be solved. So let's give it a try.

If you reached this commit through git bisect or if you just want more
information about FBC, please see:
    commit a98ee79317b4091cafb502b4ffdbbbe1335e298c
    Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
    Date:   Tue Feb 16 18:47:21 2016 -0200
        drm/i915/fbc: enable FBC by default on HSW and BDW

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Vetter March 22, 2016, 11:16 a.m. UTC | #1
On Mon, Mar 21, 2016 at 04:26:58PM -0300, Paulo Zanoni wrote:
> Now that we're more protected against user space doing frontbuffer
> mmap rendering, the last - how many times did I say this before? -
> SKL problem seems to be solved. So let's give it a try.
> 
> If you reached this commit through git bisect or if you just want more
> information about FBC, please see:
>     commit a98ee79317b4091cafb502b4ffdbbbe1335e298c
>     Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>     Date:   Tue Feb 16 18:47:21 2016 -0200
>         drm/i915/fbc: enable FBC by default on HSW and BDW
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 718ac38..67f8810 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1270,7 +1270,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  	 * know what's going on. */
>  	if (i915.enable_fbc < 0) {
>  		i915.enable_fbc = IS_HASWELL(dev_priv) ||
> -				  IS_BROADWELL(dev_priv);
> +				  IS_BROADWELL(dev_priv) ||
> +				  IS_SKYLAKE(dev_priv);

Can we just future-proof this and enable on everything gen8+ where we have
fbc? Apparently bsw/bxt simply lack this ...
-Daniel

>  		DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",
>  			      i915.enable_fbc);
>  	}
> -- 
> 2.7.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Zanoni, Paulo R March 22, 2016, 9:51 p.m. UTC | #2
Em Ter, 2016-03-22 às 12:16 +0100, Daniel Vetter escreveu:
> On Mon, Mar 21, 2016 at 04:26:58PM -0300, Paulo Zanoni wrote:

> > 

> > Now that we're more protected against user space doing frontbuffer

> > mmap rendering, the last - how many times did I say this before? -

> > SKL problem seems to be solved. So let's give it a try.

> > 

> > If you reached this commit through git bisect or if you just want

> > more

> > information about FBC, please see:

> >     commit a98ee79317b4091cafb502b4ffdbbbe1335e298c

> >     Author: Paulo Zanoni <paulo.r.zanoni@intel.com>

> >     Date:   Tue Feb 16 18:47:21 2016 -0200

> >         drm/i915/fbc: enable FBC by default on HSW and BDW

> > 

> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> > ---

> >  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-

> >  1 file changed, 2 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c

> > b/drivers/gpu/drm/i915/intel_fbc.c

> > index 718ac38..67f8810 100644

> > --- a/drivers/gpu/drm/i915/intel_fbc.c

> > +++ b/drivers/gpu/drm/i915/intel_fbc.c

> > @@ -1270,7 +1270,8 @@ void intel_fbc_init(struct drm_i915_private

> > *dev_priv)

> >  	 * know what's going on. */

> >  	if (i915.enable_fbc < 0) {

> >  		i915.enable_fbc = IS_HASWELL(dev_priv) ||

> > -				  IS_BROADWELL(dev_priv);

> > +				  IS_BROADWELL(dev_priv) ||

> > +				  IS_SKYLAKE(dev_priv);

> Can we just future-proof this and enable on everything gen8+ where we

> have

> fbc? Apparently bsw/bxt simply lack this ...


This can be done, but I'm not sure if it's a good idea, given FBC's
never-ending history of platform-specific workarounds. We'd force
people to have to have FBC working right from the start. Hmmm, that
could actually be a good thing, enforcing people to make features work.

> -Daniel

> 

> > 

> >  		DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",

> >  			      i915.enable_fbc);

> >  	}

> > -- 

> > 2.7.0

> > 

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter March 23, 2016, 8:55 a.m. UTC | #3
On Tue, Mar 22, 2016 at 09:51:34PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-03-22 às 12:16 +0100, Daniel Vetter escreveu:
> > On Mon, Mar 21, 2016 at 04:26:58PM -0300, Paulo Zanoni wrote:
> > > 
> > > Now that we're more protected against user space doing frontbuffer
> > > mmap rendering, the last - how many times did I say this before? -
> > > SKL problem seems to be solved. So let's give it a try.
> > > 
> > > If you reached this commit through git bisect or if you just want
> > > more
> > > information about FBC, please see:
> > >     commit a98ee79317b4091cafb502b4ffdbbbe1335e298c
> > >     Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >     Date:   Tue Feb 16 18:47:21 2016 -0200
> > >         drm/i915/fbc: enable FBC by default on HSW and BDW
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 718ac38..67f8810 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -1270,7 +1270,8 @@ void intel_fbc_init(struct drm_i915_private
> > > *dev_priv)
> > >  	 * know what's going on. */
> > >  	if (i915.enable_fbc < 0) {
> > >  		i915.enable_fbc = IS_HASWELL(dev_priv) ||
> > > -				  IS_BROADWELL(dev_priv);
> > > +				  IS_BROADWELL(dev_priv) ||
> > > +				  IS_SKYLAKE(dev_priv);
> > Can we just future-proof this and enable on everything gen8+ where we
> > have
> > fbc? Apparently bsw/bxt simply lack this ...
> 
> This can be done, but I'm not sure if it's a good idea, given FBC's
> never-ending history of platform-specific workarounds. We'd force
> people to have to have FBC working right from the start. Hmmm, that
> could actually be a good thing, enforcing people to make features work.

Well we generally do the same in all other places too - we just enable
everything and smash all the patches in. Then testing/power-on happens,
and more fixes pile on top.

We can still disable something if it's completely broken. But I'd really
prefer if this is a concious decision that requires explicit action.
Otherwise I fear we'll "forget" FBC again :( And e.g. modesetting or
rendering also require tons of special cases on each new platform, we
don't disable them either. We just have one overall flag for the entire
driver, which I think should be good enough.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 718ac38..67f8810 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1270,7 +1270,8 @@  void intel_fbc_init(struct drm_i915_private *dev_priv)
 	 * know what's going on. */
 	if (i915.enable_fbc < 0) {
 		i915.enable_fbc = IS_HASWELL(dev_priv) ||
-				  IS_BROADWELL(dev_priv);
+				  IS_BROADWELL(dev_priv) ||
+				  IS_SKYLAKE(dev_priv);
 		DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",
 			      i915.enable_fbc);
 	}