| Message ID | 1454318774-6086-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 01, 2016 at 01:26:14AM -0800, Matt Roper wrote: > Due to our lack of two-step watermark programming, our driver has > historically pretended that the cursor plane is always on for the > purpose of watermark calculations; this helps avoid serious flickering > when the cursor turns off/on (e.g., when the user moves the mouse > pointer to a different screen). That workaround was accidentally > dropped as we started working toward atomic watermark updates. Since we > still aren't quite there yet with two-stage updates, we need to > resurrect the workaround and treat the cursor as always active. > > Cc: simdev11@outlook.com > Cc: manfred.kitzbichler@gmail.com > Reported-by: simdev11@outlook.com > Reported-by: manfred.kitzbichler@gmail.com > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93892 > Fixes: 43d59eda1 ("drm/i915: Eliminate usage of plane_wm_parameters from ILK-style WM code (v2)") > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > I'm traveling at the moment and don't have any HW access, so this patch is > completely untested, but I think it will solve the issue being reported. > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 31bc4ea..a53c018 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -1799,16 +1799,21 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate, > const struct intel_plane_state *pstate, > uint32_t mem_value) > { > - int cpp = pstate->base.fb ? > - drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0; > + /* > + * We treat the cursor plane as always-on for the purposes of watermark > + * calculation. Until we have two-stage watermark programming merged, > + * this is necessary to avoid flickering. > + */ > + int cpp = 4; > + int width = drm_rect_width(&pstate->dst) ?: 64; I don't think we can rely on that being correct when the plane is not visible. Also for the cursor, I'm not sure we should be even using the visible width of the plane when it's partially visible. > > - if (!cstate->base.active || !pstate->visible) > + if (!cstate->base.active) > return 0; > > + > return ilk_wm_method2(ilk_pipe_pixel_rate(cstate), > cstate->base.adjusted_mode.crtc_htotal, > - drm_rect_width(&pstate->dst), > - cpp, mem_value); > + width, cpp, mem_value); > } > > /* Only for WM_LP. */ > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
I can confirm that the proposed patch fixes the flicker problem on my system (openSuse Factory kernel 4.4.0-2-default), even though I had to slightly modify it to apply it to the current openSuse Factory sources). On 02/01/2016 02:22 PM, Ville Syrjälä wrote: > On Mon, Feb 01, 2016 at 01:26:14AM -0800, Matt Roper wrote: >> Due to our lack of two-step watermark programming, our driver has >> historically pretended that the cursor plane is always on for the >> purpose of watermark calculations; this helps avoid serious flickering >> when the cursor turns off/on (e.g., when the user moves the mouse >> pointer to a different screen). That workaround was accidentally >> dropped as we started working toward atomic watermark updates. Since we >> still aren't quite there yet with two-stage updates, we need to >> resurrect the workaround and treat the cursor as always active. >> >> Cc: simdev11@outlook.com >> Cc: manfred.kitzbichler@gmail.com >> Reported-by: simdev11@outlook.com >> Reported-by: manfred.kitzbichler@gmail.com >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93892 >> Fixes: 43d59eda1 ("drm/i915: Eliminate usage of plane_wm_parameters from ILK-style WM code (v2)") >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> --- >> I'm traveling at the moment and don't have any HW access, so this patch is >> completely untested, but I think it will solve the issue being reported. >> >> drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 31bc4ea..a53c018 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -1799,16 +1799,21 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate, >> const struct intel_plane_state *pstate, >> uint32_t mem_value) >> { >> - int cpp = pstate->base.fb ? >> - drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0; >> + /* >> + * We treat the cursor plane as always-on for the purposes of watermark >> + * calculation. Until we have two-stage watermark programming merged, >> + * this is necessary to avoid flickering. >> + */ >> + int cpp = 4; >> + int width = drm_rect_width(&pstate->dst) ?: 64; > I don't think we can rely on that being correct when the plane is not > visible. Also for the cursor, I'm not sure we should be even using the > visible width of the plane when it's partially visible. > >> >> - if (!cstate->base.active || !pstate->visible) >> + if (!cstate->base.active) >> return 0; >> >> + >> return ilk_wm_method2(ilk_pipe_pixel_rate(cstate), >> cstate->base.adjusted_mode.crtc_htotal, >> - drm_rect_width(&pstate->dst), >> - cpp, mem_value); >> + width, cpp, mem_value); >> } >> >> /* Only for WM_LP. */ >> -- >> 2.1.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Feb 01, 2016 at 04:22:22PM +0200, Ville Syrjälä wrote: > On Mon, Feb 01, 2016 at 01:26:14AM -0800, Matt Roper wrote: > > Due to our lack of two-step watermark programming, our driver has > > historically pretended that the cursor plane is always on for the > > purpose of watermark calculations; this helps avoid serious flickering > > when the cursor turns off/on (e.g., when the user moves the mouse > > pointer to a different screen). That workaround was accidentally > > dropped as we started working toward atomic watermark updates. Since we > > still aren't quite there yet with two-stage updates, we need to > > resurrect the workaround and treat the cursor as always active. > > > > Cc: simdev11@outlook.com > > Cc: manfred.kitzbichler@gmail.com > > Reported-by: simdev11@outlook.com > > Reported-by: manfred.kitzbichler@gmail.com > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93892 > > Fixes: 43d59eda1 ("drm/i915: Eliminate usage of plane_wm_parameters from ILK-style WM code (v2)") > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > I'm traveling at the moment and don't have any HW access, so this patch is > > completely untested, but I think it will solve the issue being reported. > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 31bc4ea..a53c018 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -1799,16 +1799,21 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate, > > const struct intel_plane_state *pstate, > > uint32_t mem_value) > > { > > - int cpp = pstate->base.fb ? > > - drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0; > > + /* > > + * We treat the cursor plane as always-on for the purposes of watermark > > + * calculation. Until we have two-stage watermark programming merged, > > + * this is necessary to avoid flickering. > > + */ > > + int cpp = 4; > > + int width = drm_rect_width(&pstate->dst) ?: 64; > > I don't think we can rely on that being correct when the plane is not > visible. Also for the cursor, I'm not sure we should be even using the > visible width of the plane when it's partially visible. > But would this cause problems? I thought higher values were always "safe" (as long as they don't go over the maximums), just non-optimal since they translate to the level of FIFO data at which we start fetching more memory. Acording to the bspec we don't even need to ever program watermarks from the values setup by the BIOS if we're willing to live with non-optimal power/memory bandwidth usage. The proper fix is definitely to re-merge the two-step watermark programming, but we still haven't tracked down why that patch upset the CI system yet (and I won't have time to look at that in depth until I'm done traveling). This seemed like a fairly safe/non-invasive temporary workaround for the user-visible issues. I'm open to other workarounds though if you have an idea for something that will work better. Thanks. Matt > > > > - if (!cstate->base.active || !pstate->visible) > > + if (!cstate->base.active) > > return 0; > > > > + > > return ilk_wm_method2(ilk_pipe_pixel_rate(cstate), > > cstate->base.adjusted_mode.crtc_htotal, > > - drm_rect_width(&pstate->dst), > > - cpp, mem_value); > > + width, cpp, mem_value); > > } > > > > /* Only for WM_LP. */ > > -- > > 2.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
On Mon, Feb 01, 2016 at 07:31:47PM -0800, Matt Roper wrote: > On Mon, Feb 01, 2016 at 04:22:22PM +0200, Ville Syrjälä wrote: > > On Mon, Feb 01, 2016 at 01:26:14AM -0800, Matt Roper wrote: > > > Due to our lack of two-step watermark programming, our driver has > > > historically pretended that the cursor plane is always on for the > > > purpose of watermark calculations; this helps avoid serious flickering > > > when the cursor turns off/on (e.g., when the user moves the mouse > > > pointer to a different screen). That workaround was accidentally > > > dropped as we started working toward atomic watermark updates. Since we > > > still aren't quite there yet with two-stage updates, we need to > > > resurrect the workaround and treat the cursor as always active. > > > > > > Cc: simdev11@outlook.com > > > Cc: manfred.kitzbichler@gmail.com > > > Reported-by: simdev11@outlook.com > > > Reported-by: manfred.kitzbichler@gmail.com > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93892 > > > Fixes: 43d59eda1 ("drm/i915: Eliminate usage of plane_wm_parameters from ILK-style WM code (v2)") > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > I'm traveling at the moment and don't have any HW access, so this patch is > > > completely untested, but I think it will solve the issue being reported. > > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 31bc4ea..a53c018 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -1799,16 +1799,21 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate, > > > const struct intel_plane_state *pstate, > > > uint32_t mem_value) > > > { > > > - int cpp = pstate->base.fb ? > > > - drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0; > > > + /* > > > + * We treat the cursor plane as always-on for the purposes of watermark > > > + * calculation. Until we have two-stage watermark programming merged, > > > + * this is necessary to avoid flickering. > > > + */ > > > + int cpp = 4; > > > + int width = drm_rect_width(&pstate->dst) ?: 64; > > > > I don't think we can rely on that being correct when the plane is not > > visible. Also for the cursor, I'm not sure we should be even using the > > visible width of the plane when it's partially visible. > > > > But would this cause problems? I thought higher values were always > "safe" (as long as they don't go over the maximums), just non-optimal > since they translate to the level of FIFO data at which we start > fetching more memory. Acording to the bspec we don't even need to ever > program watermarks from the values setup by the BIOS if we're willing to > live with non-optimal power/memory bandwidth usage. I'd probablty just make it width = visible ? crtc_w : 64; to make it close to where we were before. > > The proper fix is definitely to re-merge the two-step watermark > programming, but we still haven't tracked down why that patch upset the > CI system yet (and I won't have time to look at that in depth until I'm > done traveling). This seemed like a fairly safe/non-invasive temporary > workaround for the user-visible issues. I'm open to other workarounds > though if you have an idea for something that will work better. > > Thanks. > > > Matt > > > > > > > - if (!cstate->base.active || !pstate->visible) > > > + if (!cstate->base.active) > > > return 0; > > > > > > + > > > return ilk_wm_method2(ilk_pipe_pixel_rate(cstate), > > > cstate->base.adjusted_mode.crtc_htotal, > > > - drm_rect_width(&pstate->dst), > > > - cpp, mem_value); > > > + width, cpp, mem_value); > > > } > > > > > > /* Only for WM_LP. */ > > > -- > > > 2.1.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 31bc4ea..a53c018 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1799,16 +1799,21 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate, const struct intel_plane_state *pstate, uint32_t mem_value) { - int cpp = pstate->base.fb ? - drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0; + /* + * We treat the cursor plane as always-on for the purposes of watermark + * calculation. Until we have two-stage watermark programming merged, + * this is necessary to avoid flickering. + */ + int cpp = 4; + int width = drm_rect_width(&pstate->dst) ?: 64; - if (!cstate->base.active || !pstate->visible) + if (!cstate->base.active) return 0; + return ilk_wm_method2(ilk_pipe_pixel_rate(cstate), cstate->base.adjusted_mode.crtc_htotal, - drm_rect_width(&pstate->dst), - cpp, mem_value); + width, cpp, mem_value); } /* Only for WM_LP. */
Due to our lack of two-step watermark programming, our driver has historically pretended that the cursor plane is always on for the purpose of watermark calculations; this helps avoid serious flickering when the cursor turns off/on (e.g., when the user moves the mouse pointer to a different screen). That workaround was accidentally dropped as we started working toward atomic watermark updates. Since we still aren't quite there yet with two-stage updates, we need to resurrect the workaround and treat the cursor as always active. Cc: simdev11@outlook.com Cc: manfred.kitzbichler@gmail.com Reported-by: simdev11@outlook.com Reported-by: manfred.kitzbichler@gmail.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93892 Fixes: 43d59eda1 ("drm/i915: Eliminate usage of plane_wm_parameters from ILK-style WM code (v2)") Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- I'm traveling at the moment and don't have any HW access, so this patch is completely untested, but I think it will solve the issue being reported. drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)