diff mbox

[04/18] drm/i915: extract crtc_is_valid() on the FBC code

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

Commit Message

Zanoni, Paulo R Oct. 20, 2015, 1:49 p.m. UTC
We're going to kill intel_fbc_find_crtc(), that's why a big part of
the logic moved from intel_fbc_find_crtc() to crtc_is_valid().

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

Comments

Chris Wilson Oct. 20, 2015, 3:52 p.m. UTC | #1
On Tue, Oct 20, 2015 at 11:49:50AM -0200, Paulo Zanoni wrote:
> We're going to kill intel_fbc_find_crtc(), that's why a big part of
> the logic moved from intel_fbc_find_crtc() to crtc_is_valid().
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index b9cfd16..1162787 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -538,27 +538,33 @@ static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
>  	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
>  }
>  
> +static bool crtc_is_valid(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	enum pipe pipe = crtc->pipe;
> +
> +	if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8) &&
> +	    pipe != PIPE_A)

Keeping

if (pipe_a_only(dev_priv) && pipe != PIPE_A)
	return false;

would have been nicer.

> +		return false;
> +
> +	return intel_crtc_active(&crtc->base) &&
> +	       to_intel_plane_state(crtc->base.primary->state)->visible &&
> +	       crtc->base.primary->fb != NULL;

And then you can split this line up for a little more clarity. If you
are taking the time to refactor into a separate function for
readability, you may as well apply a little polish as well.
-Chris
Zanoni, Paulo R Oct. 21, 2015, 5:16 p.m. UTC | #2
Em Ter, 2015-10-20 às 16:52 +0100, Chris Wilson escreveu:
> On Tue, Oct 20, 2015 at 11:49:50AM -0200, Paulo Zanoni wrote:

> > We're going to kill intel_fbc_find_crtc(), that's why a big part of

> > the logic moved from intel_fbc_find_crtc() to crtc_is_valid().

> > 

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

> > ---

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

> >  1 file changed, 16 insertions(+), 10 deletions(-)

> > 

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

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

> > index b9cfd16..1162787 100644

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

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

> > @@ -538,27 +538,33 @@ static void set_no_fbc_reason(struct

> > drm_i915_private *dev_priv,

> >  	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);

> >  }

> >  

> > +static bool crtc_is_valid(struct intel_crtc *crtc)

> > +{

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

> > >dev_private;

> > +	enum pipe pipe = crtc->pipe;

> > +

> > +	if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >=

> > 8) &&

> > +	    pipe != PIPE_A)

> 

> Keeping

> 

> if (pipe_a_only(dev_priv) && pipe != PIPE_A)

> 	return false;

> 

> would have been nicer.


I can do that on v2.

> 

> > +		return false;

> > +

> > +	return intel_crtc_active(&crtc->base) &&

> > +	       to_intel_plane_state(crtc->base.primary->state)-

> > >visible &&

> > +	       crtc->base.primary->fb != NULL;

> 

> And then you can split this line up for a little more clarity. If you

> are taking the time to refactor into a separate function for

> readability, you may as well apply a little polish as well.


I really don't get what you're suggesting here. Can you please clarify?


> -Chris

>
Chris Wilson Oct. 21, 2015, 5:28 p.m. UTC | #3
On Wed, Oct 21, 2015 at 05:16:04PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2015-10-20 às 16:52 +0100, Chris Wilson escreveu:
> > On Tue, Oct 20, 2015 at 11:49:50AM -0200, Paulo Zanoni wrote:
> > > We're going to kill intel_fbc_find_crtc(), that's why a big part of
> > > the logic moved from intel_fbc_find_crtc() to crtc_is_valid().
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 26 ++++++++++++++++----------
> > >  1 file changed, 16 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index b9cfd16..1162787 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -538,27 +538,33 @@ static void set_no_fbc_reason(struct
> > > drm_i915_private *dev_priv,
> > >  	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
> > >  }
> > >  
> > > +static bool crtc_is_valid(struct intel_crtc *crtc)
> > > +{
> > > +	struct drm_i915_private *dev_priv = crtc->base.dev-
> > > >dev_private;
> > > +	enum pipe pipe = crtc->pipe;
> > > +
> > > +	if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >=
> > > 8) &&
> > > +	    pipe != PIPE_A)
> > 
> > Keeping
> > 
> > if (pipe_a_only(dev_priv) && pipe != PIPE_A)
> > 	return false;
> > 
> > would have been nicer.
> 
> I can do that on v2.
> 
> > 
> > > +		return false;
> > > +
> > > +	return intel_crtc_active(&crtc->base) &&
> > > +	       to_intel_plane_state(crtc->base.primary->state)-
> > > >visible &&
> > > +	       crtc->base.primary->fb != NULL;
> > 
> > And then you can split this line up for a little more clarity. If you
> > are taking the time to refactor into a separate function for
> > readability, you may as well apply a little polish as well.
> 
> I really don't get what you're suggesting here. Can you please clarify?

I think multiline conditionals do not add to readibilty. Since you have
already taken the effort to split the predicate out into its own
function,

  if (!intel_crtc_active(&crtc->base))
    return false;

  if (!to_intel_plane_state(crtc->base.primary->state)->visible)
    return false;

  /* both of these are used repeatedly in intel_display.c, probably worth
   * refactoring into intel_plane_active(crtc, crtc->base.primary); later
   * on
   */


  /* given the two above, this is redundant. more evidence that we should
   * not be writing this code ourselves but calling an intel_display
   * helper
   */
  if (crtc->base.primary->fb)
    return false;

  return true;
-Chris
Maarten Lankhorst Oct. 22, 2015, 7:52 a.m. UTC | #4
Op 20-10-15 om 15:49 schreef Paulo Zanoni:
> We're going to kill intel_fbc_find_crtc(), that's why a big part of
> the logic moved from intel_fbc_find_crtc() to crtc_is_valid().
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index b9cfd16..1162787 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -538,27 +538,33 @@ static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
>  	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
>  }
>  
> +static bool crtc_is_valid(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	enum pipe pipe = crtc->pipe;
> +
> +	if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8) &&
> +	    pipe != PIPE_A)
> +		return false;
> +
> +	return intel_crtc_active(&crtc->base) &&
> +	       to_intel_plane_state(crtc->base.primary->state)->visible &&
> +	       crtc->base.primary->fb != NULL;
> +}
>
I've been considering something like this, but could it be changed to take atomic states as arguments?
That way it will be easier to use when >1 flip depth is allowed in the future, and intel_crtc_active is not
a check that should be used here.

At some point in the near future I want to convert intel_unpin_work to take the previous and next crtc/plane
states, that would become a lot easier if this code would be more atomic like. :)

~Maarten
Zanoni, Paulo R Oct. 22, 2015, 7:26 p.m. UTC | #5
Em Qui, 2015-10-22 às 09:52 +0200, Maarten Lankhorst escreveu:
> Op 20-10-15 om 15:49 schreef Paulo Zanoni:

> > We're going to kill intel_fbc_find_crtc(), that's why a big part of

> > the logic moved from intel_fbc_find_crtc() to crtc_is_valid().

> > 

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

> > ---

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

> >  1 file changed, 16 insertions(+), 10 deletions(-)

> > 

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

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

> > index b9cfd16..1162787 100644

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

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

> > @@ -538,27 +538,33 @@ static void set_no_fbc_reason(struct

> > drm_i915_private *dev_priv,

> >  	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);

> >  }

> >  

> > +static bool crtc_is_valid(struct intel_crtc *crtc)

> > +{

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

> > >dev_private;

> > +	enum pipe pipe = crtc->pipe;

> > +

> > +	if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >=

> > 8) &&

> > +	    pipe != PIPE_A)

> > +		return false;

> > +

> > +	return intel_crtc_active(&crtc->base) &&

> > +	       to_intel_plane_state(crtc->base.primary->state)-

> > >visible &&

> > +	       crtc->base.primary->fb != NULL;

> > +}

> > 

> I've been considering something like this, but could it be changed to

> take atomic states as arguments?


I'm not sure. All I know is that this is still (both now and at the end
of the series) being called while the CRTC is already enabled, not
during modeset paths. The visible/invisible case seems to be the most
important check. The other two checks are like this simply because this
code was written a loooong time ago and was never updated while the
rest of the tree evolved.

Since there are so many things to update on the FBC code I was just
trying to move the code around without really over-analyzing it since
it seemed to be working. But I guess these days any line of i915 code
touched on a patch has to be perfect :)

Anyway, I'll try to update this code on this series since you & Chris &
Ville already asked about it.

> That way it will be easier to use when >1 flip depth is allowed in

> the future, and intel_crtc_active is not

> a check that should be used here.

> 

> At some point in the near future I want to convert intel_unpin_work

> to take the previous and next crtc/plane

> states, that would become a lot easier if this code would be more

> atomic like. :)

> 

> ~Maarten
Ville Syrjälä Oct. 22, 2015, 7:42 p.m. UTC | #6
On Thu, Oct 22, 2015 at 07:26:36PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-10-22 às 09:52 +0200, Maarten Lankhorst escreveu:
> > Op 20-10-15 om 15:49 schreef Paulo Zanoni:
> > > We're going to kill intel_fbc_find_crtc(), that's why a big part of
> > > the logic moved from intel_fbc_find_crtc() to crtc_is_valid().
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 26 ++++++++++++++++----------
> > >  1 file changed, 16 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index b9cfd16..1162787 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -538,27 +538,33 @@ static void set_no_fbc_reason(struct
> > > drm_i915_private *dev_priv,
> > >  	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
> > >  }
> > >  
> > > +static bool crtc_is_valid(struct intel_crtc *crtc)
> > > +{
> > > +	struct drm_i915_private *dev_priv = crtc->base.dev-
> > > >dev_private;
> > > +	enum pipe pipe = crtc->pipe;
> > > +
> > > +	if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >=
> > > 8) &&
> > > +	    pipe != PIPE_A)
> > > +		return false;
> > > +
> > > +	return intel_crtc_active(&crtc->base) &&
> > > +	       to_intel_plane_state(crtc->base.primary->state)-
> > > >visible &&
> > > +	       crtc->base.primary->fb != NULL;
> > > +}
> > > 
> > I've been considering something like this, but could it be changed to
> > take atomic states as arguments?
> 
> I'm not sure. All I know is that this is still (both now and at the end
> of the series) being called while the CRTC is already enabled, not
> during modeset paths. The visible/invisible case seems to be the most
> important check. The other two checks are like this simply because this
> code was written a loooong time ago and was never updated while the
> rest of the tree evolved.

You know, fbc_score could be correctly calculated when we are
calculating the rest of the primary plane state ;)

> 
> Since there are so many things to update on the FBC code I was just
> trying to move the code around without really over-analyzing it since
> it seemed to be working. But I guess these days any line of i915 code
> touched on a patch has to be perfect :)
> 
> Anyway, I'll try to update this code on this series since you & Chris &
> Ville already asked about it.
> 
> > That way it will be easier to use when >1 flip depth is allowed in
> > the future, and intel_crtc_active is not
> > a check that should be used here.
> > 
> > At some point in the near future I want to convert intel_unpin_work
> > to take the previous and next crtc/plane
> > states, that would become a lot easier if this code would be more
> > atomic like. :)
> > 
> > ~Maarten
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b9cfd16..1162787 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -538,27 +538,33 @@  static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
 	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
 }
 
+static bool crtc_is_valid(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	enum pipe pipe = crtc->pipe;
+
+	if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8) &&
+	    pipe != PIPE_A)
+		return false;
+
+	return intel_crtc_active(&crtc->base) &&
+	       to_intel_plane_state(crtc->base.primary->state)->visible &&
+	       crtc->base.primary->fb != NULL;
+}
+
 static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 {
 	struct drm_crtc *crtc = NULL, *tmp_crtc;
 	enum pipe pipe;
-	bool pipe_a_only = false;
-
-	if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
-		pipe_a_only = true;
 
 	for_each_pipe(dev_priv, pipe) {
 		tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 
-		if (intel_crtc_active(tmp_crtc) &&
-		    to_intel_plane_state(tmp_crtc->primary->state)->visible)
+		if (crtc_is_valid(to_intel_crtc(tmp_crtc)))
 			crtc = tmp_crtc;
-
-		if (pipe_a_only)
-			break;
 	}
 
-	if (!crtc || crtc->primary->fb == NULL)
+	if (!crtc)
 		return NULL;
 
 	return crtc;