| Message ID | 1445349004-16409-6-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 20, 2015 at 11:49:51AM -0200, Paulo Zanoni wrote: > This thing where we need to get the crtc either from the work > structure or the fbc structure itself is confusing and unnecessary. > Set fbc.crtc right when scheduling the enable work so we can always > use it. Pardon? It was confusing to have the crtc passed along with the work item as opposed to digging it out from an indirect link on the dev_priv. iirc work->crtc was part of the serialisation of the work item. What I think you meant was that you were passing around the wrong struct, e.g. intel_fbc_enable() just wants the crtc, in intel_fbc_flip_prepare() you check for the work struct, and then you should just use the work struct. -Chris
Em Ter, 2015-10-20 às 17:03 +0100, Chris Wilson escreveu: > On Tue, Oct 20, 2015 at 11:49:51AM -0200, Paulo Zanoni wrote: > > This thing where we need to get the crtc either from the work > > structure or the fbc structure itself is confusing and unnecessary. > > Set fbc.crtc right when scheduling the enable work so we can always > > use it. > > Pardon? It was confusing to have the crtc passed along with the work > item as opposed to digging it out from an indirect link on the > dev_priv. > iirc work->crtc was part of the serialisation of the work item. > > What I think you meant was that you were passing around the wrong > struct, e.g. > intel_fbc_enable() just wants the crtc, in intel_fbc_flip_prepare() > you > check for the work struct, and then you should just use the work > struct. The problem is not what gets passed and how to retrieve it. The problem is that when we're in the other parts of the code we always have to keep in mind that if FBC is already enabled we have to get the CRTC from place A, if FBC is scheduled we have to get the CRTC from place B, and if it's disabled there's no CRTC. Having a single place to retrieve the CRTC from allows us to treat the "is enabled" and "is scheduled" cases as the same case, reducing the mistake surface. I guess I should add this to the commit message. Anyway, it's worth mentioning that later on this series when we introduce enable() and disable() we already set dev_priv->fbc.crtc at enable(), so all the activate/deactivate/update code can just look at that regardless of the current state. So this patch is just an intermediate step to keep everything simple and working until the enable/disable patch. > -Chris >
On Wed, Oct 21, 2015 at 05:27:32PM +0000, Zanoni, Paulo R wrote: > Em Ter, 2015-10-20 às 17:03 +0100, Chris Wilson escreveu: > > On Tue, Oct 20, 2015 at 11:49:51AM -0200, Paulo Zanoni wrote: > > > This thing where we need to get the crtc either from the work > > > structure or the fbc structure itself is confusing and unnecessary. > > > Set fbc.crtc right when scheduling the enable work so we can always > > > use it. > > > > Pardon? It was confusing to have the crtc passed along with the work > > item as opposed to digging it out from an indirect link on the > > dev_priv. > > iirc work->crtc was part of the serialisation of the work item. > > > > What I think you meant was that you were passing around the wrong > > struct, e.g. > > intel_fbc_enable() just wants the crtc, in intel_fbc_flip_prepare() > > you > > check for the work struct, and then you should just use the work > > struct. > > The problem is not what gets passed and how to retrieve it. The problem > is that when we're in the other parts of the code we always have to > keep in mind that if FBC is already enabled we have to get the CRTC > from place A, if FBC is scheduled we have to get the CRTC from place B, > and if it's disabled there's no CRTC. Having a single place to retrieve > the CRTC from allows us to treat the "is enabled" and "is scheduled" > cases as the same case, reducing the mistake surface. I guess I should > add this to the commit message. Sure that's also my argument why I don't like this particular patch as is. :) The functions here had just one place to retreive everything they needed, and now they need two. I guess the way to make us both happy is to embed the work_struct inside the fbc struct - and that way you can eliminate the divergence. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f04f56f..2cb7a05 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -921,7 +921,6 @@ struct i915_fbc { struct intel_fbc_work { struct delayed_work work; - struct intel_crtc *crtc; struct drm_framebuffer *fb; } *fbc_work; diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 1162787..dd66649 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -385,14 +385,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv) return dev_priv->fbc.enabled; } -static void intel_fbc_enable(struct intel_crtc *crtc, - const struct drm_framebuffer *fb) +static void intel_fbc_enable(const struct drm_framebuffer *fb) { - struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + struct drm_i915_private *dev_priv = fb->dev->dev_private; + struct intel_crtc *crtc = dev_priv->fbc.crtc; dev_priv->fbc.enable_fbc(crtc); - dev_priv->fbc.crtc = crtc; dev_priv->fbc.fb_id = fb->base.id; dev_priv->fbc.y = crtc->base.y; } @@ -402,8 +401,8 @@ static void intel_fbc_work_fn(struct work_struct *__work) struct intel_fbc_work *work = container_of(to_delayed_work(__work), struct intel_fbc_work, work); - struct drm_i915_private *dev_priv = work->crtc->base.dev->dev_private; - struct drm_framebuffer *crtc_fb = work->crtc->base.primary->fb; + struct drm_i915_private *dev_priv = work->fb->dev->dev_private; + struct drm_framebuffer *crtc_fb = dev_priv->fbc.crtc->base.primary->fb; mutex_lock(&dev_priv->fbc.lock); if (work == dev_priv->fbc.fbc_work) { @@ -411,7 +410,7 @@ static void intel_fbc_work_fn(struct work_struct *__work) * the prior work. */ if (crtc_fb == work->fb) - intel_fbc_enable(work->crtc, work->fb); + intel_fbc_enable(work->fb); dev_priv->fbc.fbc_work = NULL; } @@ -453,15 +452,15 @@ static void intel_fbc_schedule_enable(struct intel_crtc *crtc) WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); intel_fbc_cancel_work(dev_priv); + dev_priv->fbc.crtc = crtc; work = kzalloc(sizeof(*work), GFP_KERNEL); if (work == NULL) { DRM_ERROR("Failed to allocate FBC work structure\n"); - intel_fbc_enable(crtc, crtc->base.primary->fb); + intel_fbc_enable(crtc->base.primary->fb); return; } - work->crtc = crtc; work->fb = crtc->base.primary->fb; INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn); @@ -1059,11 +1058,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, mutex_lock(&dev_priv->fbc.lock); - if (dev_priv->fbc.enabled) + if (dev_priv->fbc.enabled || dev_priv->fbc.fbc_work) fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe); - else if (dev_priv->fbc.fbc_work) - fbc_bits = INTEL_FRONTBUFFER_PRIMARY( - dev_priv->fbc.fbc_work->crtc->pipe); else fbc_bits = dev_priv->fbc.possible_framebuffer_bits; @@ -1119,8 +1115,7 @@ void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv, if (fbc_bits & frontbuffer_bits) dev_priv->fbc.flip_prepare(dev_priv); } else if (dev_priv->fbc.fbc_work) { - fbc_bits = INTEL_FRONTBUFFER_PRIMARY( - dev_priv->fbc.fbc_work->crtc->pipe); + fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe); if (fbc_bits & frontbuffer_bits) __intel_fbc_disable(dev_priv); }
This thing where we need to get the crtc either from the work structure or the fbc structure itself is confusing and unnecessary. Set fbc.crtc right when scheduling the enable work so we can always use it. Besides the immediate advantages, this is also going to make one of the next commits much simpler. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_fbc.c | 25 ++++++++++--------------- 2 files changed, 10 insertions(+), 16 deletions(-)