| Message ID | 1445349004-16409-5-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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
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
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 --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;
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(-)