diff mbox

[19/26] drm/i915: move adjusted_mode checks from fbc_update to fbc_enable

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

Commit Message

Zanoni, Paulo R Oct. 27, 2015, 4:50 p.m. UTC 
These things can't change without a full modeset.

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

Comments

Maarten Lankhorst Oct. 29, 2015, 12:59 p.m. UTC | #1 
Op 27-10-15 om 17:50 schreef Paulo Zanoni:
> These things can't change without a full modeset.
False! Fastset can update parameters too. Although I don't think it currently prevents DBLSCAN updates,
so maybe make sure cfb enable/disable is called when updating pipe too?
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 0ba25b9..6aa9af8 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -825,7 +825,6 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  	struct drm_framebuffer *fb;
>  	struct drm_i915_gem_object *obj;
> -	const struct drm_display_mode *adjusted_mode;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
>  
> @@ -844,13 +843,6 @@ static void __intel_fbc_update(struct intel_crtc *crtc)
>  
>  	fb = crtc->base.primary->fb;
>  	obj = intel_fb_obj(fb);
> -	adjusted_mode = &crtc->config->base.adjusted_mode;
> -
> -	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
> -	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
> -		set_no_fbc_reason(dev_priv, "incompatible mode");
> -		goto out_disable;
> -	}
>  
>  	if (!intel_fbc_hw_tracking_covers_screen(crtc)) {
>  		set_no_fbc_reason(dev_priv, "mode too large for compression");
> @@ -1076,6 +1068,8 @@ void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
>  void intel_fbc_enable(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	const struct drm_display_mode *adjusted_mode =
> +					&crtc->config->base.adjusted_mode;
>  
>  	if (!fbc_supported(dev_priv))
>  		return;
> @@ -1110,6 +1104,12 @@ void intel_fbc_enable(struct intel_crtc *crtc)
>  		goto out;
>  	}
>  
> +	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
> +	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
> +		set_no_fbc_reason(dev_priv, "incompatible mode");
> +		goto out;
> +	}
> +
>  	if (intel_fbc_alloc_cfb(crtc)) {
>  		set_no_fbc_reason(dev_priv, "not enough stolen memory");
>  		goto out;
Zanoni, Paulo R Oct. 29, 2015, 5:58 p.m. UTC | #2 
Em Qui, 2015-10-29 às 13:59 +0100, Maarten Lankhorst escreveu:
> Op 27-10-15 om 17:50 schreef Paulo Zanoni:

> > These things can't change without a full modeset.

> False! Fastset can update parameters too. Although I don't think it

> currently prevents DBLSCAN updates,

> so maybe make sure cfb enable/disable is called when updating pipe

> too?


You mean that if we change for an interlaced (or double-scanned) to a
non-interlaced (or non-double-scanned) mode we won't get
crtc_disable+crtc_enable? That's surprising. I'll take a better look
here and try to write a test case. Is this just through some specific
API (like drmModeSetPlane)?

I'm trying to get test cases for every single bug I discover or
introduce.

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

> > ---

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

> >  1 file changed, 8 insertions(+), 8 deletions(-)

> > 

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

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

> > index 0ba25b9..6aa9af8 100644

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

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

> > @@ -825,7 +825,6 @@ static void __intel_fbc_update(struct

> > intel_crtc *crtc)

> >  	struct drm_i915_private *dev_priv = crtc->base.dev-

> > >dev_private;

> >  	struct drm_framebuffer *fb;

> >  	struct drm_i915_gem_object *obj;

> > -	const struct drm_display_mode *adjusted_mode;

> >  

> >  	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));

> >  

> > @@ -844,13 +843,6 @@ static void __intel_fbc_update(struct

> > intel_crtc *crtc)

> >  

> >  	fb = crtc->base.primary->fb;

> >  	obj = intel_fb_obj(fb);

> > -	adjusted_mode = &crtc->config->base.adjusted_mode;

> > -

> > -	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||

> > -	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {

> > -		set_no_fbc_reason(dev_priv, "incompatible mode");

> > -		goto out_disable;

> > -	}

> >  

> >  	if (!intel_fbc_hw_tracking_covers_screen(crtc)) {

> >  		set_no_fbc_reason(dev_priv, "mode too large for

> > compression");

> > @@ -1076,6 +1068,8 @@ void intel_fbc_flip_prepare(struct

> > drm_i915_private *dev_priv,

> >  void intel_fbc_enable(struct intel_crtc *crtc)

> >  {

> >  	struct drm_i915_private *dev_priv = crtc->base.dev-

> > >dev_private;

> > +	const struct drm_display_mode *adjusted_mode =

> > +					&crtc->config-

> > >base.adjusted_mode;

> >  

> >  	if (!fbc_supported(dev_priv))

> >  		return;

> > @@ -1110,6 +1104,12 @@ void intel_fbc_enable(struct intel_crtc

> > *crtc)

> >  		goto out;

> >  	}

> >  

> > +	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||

> > +	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {

> > +		set_no_fbc_reason(dev_priv, "incompatible mode");

> > +		goto out;

> > +	}

> > +

> >  	if (intel_fbc_alloc_cfb(crtc)) {

> >  		set_no_fbc_reason(dev_priv, "not enough stolen

> > memory");

> >  		goto out;

>
Maarten Lankhorst Nov. 2, 2015, 8:53 a.m. UTC | #3 
Op 29-10-15 om 18:58 schreef Zanoni, Paulo R:
> Em Qui, 2015-10-29 às 13:59 +0100, Maarten Lankhorst escreveu:
>> Op 27-10-15 om 17:50 schreef Paulo Zanoni:
>>> These things can't change without a full modeset.
>> False! Fastset can update parameters too. Although I don't think it
>> currently prevents DBLSCAN updates,
>> so maybe make sure cfb enable/disable is called when updating pipe
>> too?
> You mean that if we change for an interlaced (or double-scanned) to a
> non-interlaced (or non-double-scanned) mode we won't get
> crtc_disable+crtc_enable? That's surprising. I'll take a better look
> here and try to write a test case. Is this just through some specific
> API (like drmModeSetPlane)?
>
> I'm trying to get test cases for every single bug I discover or
> introduce.
>
Yeah if it does happen it's a bug though. :)

~Maarten
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 0ba25b9..6aa9af8 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -825,7 +825,6 @@  static void __intel_fbc_update(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
-	const struct drm_display_mode *adjusted_mode;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
@@ -844,13 +843,6 @@  static void __intel_fbc_update(struct intel_crtc *crtc)
 
 	fb = crtc->base.primary->fb;
 	obj = intel_fb_obj(fb);
-	adjusted_mode = &crtc->config->base.adjusted_mode;
-
-	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
-	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
-		set_no_fbc_reason(dev_priv, "incompatible mode");
-		goto out_disable;
-	}
 
 	if (!intel_fbc_hw_tracking_covers_screen(crtc)) {
 		set_no_fbc_reason(dev_priv, "mode too large for compression");
@@ -1076,6 +1068,8 @@  void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
 void intel_fbc_enable(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	const struct drm_display_mode *adjusted_mode =
+					&crtc->config->base.adjusted_mode;
 
 	if (!fbc_supported(dev_priv))
 		return;
@@ -1110,6 +1104,12 @@  void intel_fbc_enable(struct intel_crtc *crtc)
 		goto out;
 	}
 
+	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
+	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
+		set_no_fbc_reason(dev_priv, "incompatible mode");
+		goto out;
+	}
+
 	if (intel_fbc_alloc_cfb(crtc)) {
 		set_no_fbc_reason(dev_priv, "not enough stolen memory");
 		goto out;