| Message ID | 1453210558-7875-9-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Paulo,
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160119]
[cannot apply to v4.4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Paulo-Zanoni/FBC-crtc-fb-locking-smaller-fixes/20160119-214108
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-01180513 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_display.c: In function 'intel_plane_atomic_calc_changes':
>> drivers/gpu/drm/i915/intel_display.c:11811:27: warning: unused variable 'dev_priv' [-Wunused-variable]
struct drm_i915_private *dev_priv = dev->dev_private;
^
vim +/dev_priv +11811 drivers/gpu/drm/i915/intel_display.c
d21fbe87 Matt Roper 2015-09-24 11795 int src_w = drm_rect_width(&state->src) >> 16;
d21fbe87 Matt Roper 2015-09-24 11796 int src_h = drm_rect_height(&state->src) >> 16;
d21fbe87 Matt Roper 2015-09-24 11797 int dst_w = drm_rect_width(&state->dst);
d21fbe87 Matt Roper 2015-09-24 11798 int dst_h = drm_rect_height(&state->dst);
d21fbe87 Matt Roper 2015-09-24 11799
d21fbe87 Matt Roper 2015-09-24 11800 return (src_w != dst_w || src_h != dst_h);
d21fbe87 Matt Roper 2015-09-24 11801 }
d21fbe87 Matt Roper 2015-09-24 11802
da20eabd Maarten Lankhorst 2015-06-15 11803 int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
da20eabd Maarten Lankhorst 2015-06-15 11804 struct drm_plane_state *plane_state)
da20eabd Maarten Lankhorst 2015-06-15 11805 {
ab1d3a0e Maarten Lankhorst 2015-11-19 11806 struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
da20eabd Maarten Lankhorst 2015-06-15 11807 struct drm_crtc *crtc = crtc_state->crtc;
da20eabd Maarten Lankhorst 2015-06-15 11808 struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
da20eabd Maarten Lankhorst 2015-06-15 11809 struct drm_plane *plane = plane_state->plane;
da20eabd Maarten Lankhorst 2015-06-15 11810 struct drm_device *dev = crtc->dev;
da20eabd Maarten Lankhorst 2015-06-15 @11811 struct drm_i915_private *dev_priv = dev->dev_private;
da20eabd Maarten Lankhorst 2015-06-15 11812 struct intel_plane_state *old_plane_state =
da20eabd Maarten Lankhorst 2015-06-15 11813 to_intel_plane_state(plane->state);
da20eabd Maarten Lankhorst 2015-06-15 11814 int idx = intel_crtc->base.base.id, ret;
da20eabd Maarten Lankhorst 2015-06-15 11815 int i = drm_plane_index(plane);
da20eabd Maarten Lankhorst 2015-06-15 11816 bool mode_changed = needs_modeset(crtc_state);
da20eabd Maarten Lankhorst 2015-06-15 11817 bool was_crtc_enabled = crtc->state->active;
da20eabd Maarten Lankhorst 2015-06-15 11818 bool is_crtc_enabled = crtc_state->active;
da20eabd Maarten Lankhorst 2015-06-15 11819 bool turn_off, turn_on, visible, was_visible;
:::::: The code at line 11811 was first introduced by commit
:::::: da20eabd2c69761f9dfd849985eb299e3335531f drm/i915: Split plane updates of crtc->atomic into a helper, v2.
:::::: TO: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
:::::: CC: Daniel Vetter <daniel.vetter@ffwll.ch>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Op 19-01-16 om 14:35 schreef Paulo Zanoni: > We unconditionally disable/update FBC even during the page flip > IOCTLs, and an unconditional disable/update at every atomic commit > touching the primary plane shouldn't impact PC state residency > noticeably. Besides, the code that checks for rotation is a good hint > that we may be forgetting something else, so let's leave all the > decisions to intel_fbc.c, making the code much safer. > > Once we have the code to properly make FBC enable/update decisions > based on atomic states, with proper locking, then we'll be able to > evaluate whether it will be worth trying to optimize the cases where a > disable isn't needed. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> I would rather have this patch remove those 2 members entirely, but I can work with this for now. Could nuke at least disable_fbc though, being redundant with update_fbc. :) ~Maarten
Em Qui, 2016-01-21 às 14:04 +0100, Maarten Lankhorst escreveu: > Op 19-01-16 om 14:35 schreef Paulo Zanoni: > > We unconditionally disable/update FBC even during the page flip > > IOCTLs, and an unconditional disable/update at every atomic commit > > touching the primary plane shouldn't impact PC state residency > > noticeably. Besides, the code that checks for rotation is a good > > hint > > that we may be forgetting something else, so let's leave all the > > decisions to intel_fbc.c, making the code much safer. > > > > Once we have the code to properly make FBC enable/update decisions > > based on atomic states, with proper locking, then we'll be able to > > evaluate whether it will be worth trying to optimize the cases > > where a > > disable isn't needed. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > I would rather have this patch remove those 2 members entirely, but I > can work with this for now. And what would be the new way to know whether a given atomic commit touches the primary plane of a given crtc? > > Could nuke at least disable_fbc though, being redundant with > update_fbc. :) Check patch 11 :) > > ~Maarten
Op 21-01-16 om 14:27 schreef Zanoni, Paulo R: > Em Qui, 2016-01-21 às 14:04 +0100, Maarten Lankhorst escreveu: >> Op 19-01-16 om 14:35 schreef Paulo Zanoni: >>> We unconditionally disable/update FBC even during the page flip >>> IOCTLs, and an unconditional disable/update at every atomic commit >>> touching the primary plane shouldn't impact PC state residency >>> noticeably. Besides, the code that checks for rotation is a good >>> hint >>> that we may be forgetting something else, so let's leave all the >>> decisions to intel_fbc.c, making the code much safer. >>> >>> Once we have the code to properly make FBC enable/update decisions >>> based on atomic states, with proper locking, then we'll be able to >>> evaluate whether it will be worth trying to optimize the cases >>> where a >>> disable isn't needed. >>> >>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> I would rather have this patch remove those 2 members entirely, but I >> can work with this for now. > And what would be the new way to know whether a given atomic commit > touches the primary plane of a given crtc? if (drm_atomic_get_existing_plane_state(old_crtc_state->state, crtc->primary)) >> Could nuke at least disable_fbc though, being redundant with >> update_fbc. :) > Check patch 11 :) > >> ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f026ade..baab41046 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11928,6 +11928,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, case DRM_PLANE_TYPE_PRIMARY: intel_crtc->atomic.pre_disable_primary = turn_off; intel_crtc->atomic.post_enable_primary = turn_on; + intel_crtc->atomic.disable_fbc = true; + intel_crtc->atomic.update_fbc = true; if (turn_off) { /* @@ -11939,28 +11941,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, * disable. */ intel_crtc->atomic.disable_ips = true; - - intel_crtc->atomic.disable_fbc = true; } /* - * FBC does not work on some platforms for rotated - * planes, so disable it when rotation is not 0 and - * update it when rotation is set back to 0. - * - * FIXME: This is redundant with the fbc update done in - * the primary plane enable function except that that - * one is done too late. We eventually need to unify - * this. - */ - - if (visible && - INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && - dev_priv->fbc.crtc == intel_crtc && - plane_state->rotation != BIT(DRM_ROTATE_0)) - intel_crtc->atomic.disable_fbc = true; - - /* * BDW signals flip done immediately if the plane * is disabled, even if the plane enable is already * armed to occur at the next vblank :( @@ -11968,7 +11951,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, if (turn_on && IS_BROADWELL(dev)) intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.update_fbc |= visible || mode_changed; break; case DRM_PLANE_TYPE_CURSOR: break;
We unconditionally disable/update FBC even during the page flip IOCTLs, and an unconditional disable/update at every atomic commit touching the primary plane shouldn't impact PC state residency noticeably. Besides, the code that checks for rotation is a good hint that we may be forgetting something else, so let's leave all the decisions to intel_fbc.c, making the code much safer. Once we have the code to properly make FBC enable/update decisions based on atomic states, with proper locking, then we'll be able to evaluate whether it will be worth trying to optimize the cases where a disable isn't needed. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-)