| Message ID | 1445349004-16409-8-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 20, 2015 at 11:49:53AM -0200, Paulo Zanoni wrote: > Don't try to list in comments the cases where we should enable or > disable FBC: it varies a lot with the hardware generations and the > code should be the documentation. Also notice that there's already a > huge gap between the comments and what's in the code. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_fbc.c | 26 ++------------------------ > 1 file changed, 2 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index e856304..5fa4ce4 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -853,20 +853,8 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > * __intel_fbc_update - enable/disable FBC as needed, unlocked > * @dev_priv: i915 device instance > * > - * Set up the framebuffer compression hardware at mode set time. We > - * enable it if possible: > - * - plane A only (on pre-965) > - * - no pixel mulitply/line duplication > - * - no alpha buffer discard > - * - no dual wide > - * - framebuffer <= max_hdisplay in width, max_vdisplay in height > - * > - * We can't assume that any compression will take place (worst case), > - * so the compressed buffer has to be the same size as the uncompressed > - * one. It also must reside (along with the line length buffer) in > - * stolen memory. > - * > - * We need to enable/disable FBC on a global basis. > + * This function completely reevaluates the status of FBC, then enables, > + * disables or maintains it on the same state. By the end, this comment is not strictly true ,right? Since you move some of the status checking into modeset enable paths. Could you refine the function comment? -Chris
Em Qua, 2015-10-21 às 13:37 +0100, Chris Wilson escreveu: > On Tue, Oct 20, 2015 at 11:49:53AM -0200, Paulo Zanoni wrote: > > Don't try to list in comments the cases where we should enable or > > disable FBC: it varies a lot with the hardware generations and the > > code should be the documentation. Also notice that there's already > > a > > huge gap between the comments and what's in the code. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 26 ++------------------------ > > 1 file changed, 2 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > b/drivers/gpu/drm/i915/intel_fbc.c > > index e856304..5fa4ce4 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -853,20 +853,8 @@ static bool > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > * __intel_fbc_update - enable/disable FBC as needed, unlocked > > * @dev_priv: i915 device instance > > * > > - * Set up the framebuffer compression hardware at mode set > > time. We > > - * enable it if possible: > > - * - plane A only (on pre-965) > > - * - no pixel mulitply/line duplication > > - * - no alpha buffer discard > > - * - no dual wide > > - * - framebuffer <= max_hdisplay in width, max_vdisplay in > > height > > - * > > - * We can't assume that any compression will take place (worst > > case), > > - * so the compressed buffer has to be the same size as the > > uncompressed > > - * one. It also must reside (along with the line length buffer) > > in > > - * stolen memory. > > - * > > - * We need to enable/disable FBC on a global basis. > > + * This function completely reevaluates the status of FBC, then > > enables, > > + * disables or maintains it on the same state. > > By the end, this comment is not strictly true ,right? Since you move > some of the status checking into modeset enable paths. Could you > refine > the function comment? I only move the status checking into modeset enable paths on patch 12. The comment above is modified on patch 12 to reflect activate/deactivate instead of enable/disable. > -Chris >
On Wed, Oct 21, 2015 at 05:32:16PM +0000, Zanoni, Paulo R wrote: > Em Qua, 2015-10-21 às 13:37 +0100, Chris Wilson escreveu: > > On Tue, Oct 20, 2015 at 11:49:53AM -0200, Paulo Zanoni wrote: > > > Don't try to list in comments the cases where we should enable or > > > disable FBC: it varies a lot with the hardware generations and the > > > code should be the documentation. Also notice that there's already > > > a > > > huge gap between the comments and what's in the code. > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_fbc.c | 26 ++------------------------ > > > 1 file changed, 2 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > index e856304..5fa4ce4 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > @@ -853,20 +853,8 @@ static bool > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > > * __intel_fbc_update - enable/disable FBC as needed, unlocked > > > * @dev_priv: i915 device instance > > > * > > > - * Set up the framebuffer compression hardware at mode set > > > time. We > > > - * enable it if possible: > > > - * - plane A only (on pre-965) > > > - * - no pixel mulitply/line duplication > > > - * - no alpha buffer discard > > > - * - no dual wide > > > - * - framebuffer <= max_hdisplay in width, max_vdisplay in > > > height > > > - * > > > - * We can't assume that any compression will take place (worst > > > case), > > > - * so the compressed buffer has to be the same size as the > > > uncompressed > > > - * one. It also must reside (along with the line length buffer) > > > in > > > - * stolen memory. > > > - * > > > - * We need to enable/disable FBC on a global basis. > > > + * This function completely reevaluates the status of FBC, then > > > enables, > > > + * disables or maintains it on the same state. > > > > By the end, this comment is not strictly true ,right? Since you move > > some of the status checking into modeset enable paths. Could you > > refine > > the function comment? > > I only move the status checking into modeset enable paths on patch 12. > The comment above is modified on patch 12 to reflect > activate/deactivate instead of enable/disable. Honestly, I would just squash this patch, or apply it after the moving the code to correctly reflect the split in work between update/enable. -Chris
Em Qua, 2015-10-21 às 18:38 +0100, chris@chris-wilson.co.uk escreveu: > On Wed, Oct 21, 2015 at 05:32:16PM +0000, Zanoni, Paulo R wrote: > > Em Qua, 2015-10-21 às 13:37 +0100, Chris Wilson escreveu: > > > On Tue, Oct 20, 2015 at 11:49:53AM -0200, Paulo Zanoni wrote: > > > > Don't try to list in comments the cases where we should enable > > > > or > > > > disable FBC: it varies a lot with the hardware generations and > > > > the > > > > code should be the documentation. Also notice that there's > > > > already > > > > a > > > > huge gap between the comments and what's in the code. > > > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_fbc.c | 26 ++-------------------- > > > > ---- > > > > 1 file changed, 2 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > > index e856304..5fa4ce4 100644 > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > > @@ -853,20 +853,8 @@ static bool > > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > > > * __intel_fbc_update - enable/disable FBC as needed, unlocked > > > > * @dev_priv: i915 device instance > > > > * > > > > - * Set up the framebuffer compression hardware at mode set > > > > time. We > > > > - * enable it if possible: > > > > - * - plane A only (on pre-965) > > > > - * - no pixel mulitply/line duplication > > > > - * - no alpha buffer discard > > > > - * - no dual wide > > > > - * - framebuffer <= max_hdisplay in width, max_vdisplay in > > > > height > > > > - * > > > > - * We can't assume that any compression will take place (worst > > > > case), > > > > - * so the compressed buffer has to be the same size as the > > > > uncompressed > > > > - * one. It also must reside (along with the line length > > > > buffer) > > > > in > > > > - * stolen memory. > > > > - * > > > > - * We need to enable/disable FBC on a global basis. > > > > + * This function completely reevaluates the status of FBC, > > > > then > > > > enables, > > > > + * disables or maintains it on the same state. > > > > > > By the end, this comment is not strictly true ,right? Since you > > > move > > > some of the status checking into modeset enable paths. Could you > > > refine > > > the function comment? > > > > I only move the status checking into modeset enable paths on patch > > 12. > > The comment above is modified on patch 12 to reflect > > activate/deactivate instead of enable/disable. > > Honestly, I would just squash this patch, or apply it after the > moving > the code to correctly reflect the split in work between > update/enable. The way I see, removing obsolete comments that list the reasons to enable/disable FBC is orthogonal to the changes regarding enable/disable/deactivate/activate. Changing the patch order as you suggested will cause some rebase pain just to achieve the same end result later, so unless there's some strong objection I'll spare myself the pain. > -Chris >
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index e856304..5fa4ce4 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -853,20 +853,8 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) * __intel_fbc_update - enable/disable FBC as needed, unlocked * @dev_priv: i915 device instance * - * Set up the framebuffer compression hardware at mode set time. We - * enable it if possible: - * - plane A only (on pre-965) - * - no pixel mulitply/line duplication - * - no alpha buffer discard - * - no dual wide - * - framebuffer <= max_hdisplay in width, max_vdisplay in height - * - * We can't assume that any compression will take place (worst case), - * so the compressed buffer has to be the same size as the uncompressed - * one. It also must reside (along with the line length buffer) in - * stolen memory. - * - * We need to enable/disable FBC on a global basis. + * This function completely reevaluates the status of FBC, then enables, + * disables or maintains it on the same state. */ static void __intel_fbc_update(struct drm_i915_private *dev_priv) { @@ -878,7 +866,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); - /* disable framebuffer compression in vGPU */ if (intel_vgpu_active(dev_priv->dev)) i915.enable_fbc = 0; @@ -892,15 +879,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) goto out_disable; } - /* - * If FBC is already on, we just have to verify that we can - * keep it that way... - * Need to disable if: - * - more than one pipe is active - * - changing FBC params (stride, fence, mode) - * - new fb is too large to fit in compressed buffer - * - going to an unsupported config (interlace, pixel multiply, etc.) - */ drm_crtc = intel_fbc_find_crtc(dev_priv); if (!drm_crtc) { set_no_fbc_reason(dev_priv, "no output");
Don't try to list in comments the cases where we should enable or disable FBC: it varies a lot with the hardware generations and the code should be the documentation. Also notice that there's already a huge gap between the comments and what's in the code. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-)