On 02/07/2017 04:35 PM, Ville Syrjälä wrote:
> On Tue, Feb 07, 2017 at 05:03:09PM +0200, Martin Peres wrote:
>> On 07/02/17 15:22, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On 02/01/2017 03:37 PM, Ville Syrjälä wrote:
>>>> On Wed, Feb 01, 2017 at 01:41:08PM +0100, Maarten Lankhorst wrote:
>>>>> Op 31-01-17 om 20:13 schreef Uwe Kleine-König:
>>>>>> On Tue, Jan 31, 2017 at 10:03:26AM +0100, Maarten Lankhorst wrote:
>>>>>>> Op 31-01-17 om 09:09 schreef Uwe Kleine-König:
>>>>>>> Just curious, does this help?
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> index ae2c0bb4b2e8..13de4c526ca6 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> @@ -1838,7 +1838,7 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
>>>>>>> * this is necessary to avoid flickering.
>>>>>>> */
>>>>>>> int cpp = 4;
>>>>>>> - int width = pstate->base.visible ? pstate->base.crtc_w : 64;
>>>>>>> + int width = 256;
>>>>>>>
>>>>>>> if (!cstate->base.active)
>>>>>>> return 0;
>>>>>>>
>>>>>> On a kernel with this patch applied I cannot reproduce the flickering. I
>>>>>> keep that kernel running but expect that it also fixes the crash.
>>>>>
>>>>> Ok that's good news.
>>>>>
>>>>> Maybe ville or matt can comment whether this patch is the right fix?
>>>>
>>>> Well, it's just extending the hack even further. The right fix would be
>>>> to fix the wm programming sequence to respect the frame boundaries
>>>> correctly (ie. my old vblank based wm stuff).
>>>
>>> so I wonder how this goes forward. The situation seems to be well
>>> understood, but other than testing patches I don't know what to do (and
>>> there is currently no patch to test).
>>>
>>> Best regards
>>> Uwe
>>>
>>
>> The way I understand this is that no-one wants to restrict the
>> capabilities exposed by the kernel and would like a proper fix for this.
>> However, I agree with Uwe, given the low priority status of Gen5 (people
>> would rather work on hw that is used by a lot of people), we should
>> probably accept the patch proposed by Maarten as it fixes someone's
>> workflow and does not regress anything meaningful.
>
> The same code is used for ILK-BDW, so it's not just ILK. And other other
> platform suffers from the same problem of cursor vs. watermarks. It just
> seems that most people are lucky enough to not be seriously affected by
> this problem.
>
> Also it can regress some things, at least theoretically. Power consumption
> with < 256x256 for one, and potentially it could also end up rejecting
> some display modes that previously used to work with smaller cursor
> sizes (or no cursors). That last part may not be 100% true, but I was
> too lazy to go through the math to see if the cursor FIFO could end up
> being the limiting factor in some cases.
>
> I was thinking Maarten's intel_legacy_cursor_update() hack should have
> "fixed" this, but now I'm not sure since it still sets the
> legacy_cursor_update flag in the slow path, and the commit message
> didn't quite manage to tell me what the purpose of this function
> was supposed to be. The logic for picking the slow path also seems a
> little wonky to me (assuming I deduced the purpose of the function
> correctly).
>
> So, we might want something like:
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 88689a0b4183..307ee4f7bd58 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15053,8 +15053,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> old_plane_state->src_h != src_h ||
> old_plane_state->crtc_w != crtc_w ||
> old_plane_state->crtc_h != crtc_h ||
> - !old_plane_state->visible ||
> - old_plane_state->fb->modifier != fb->modifier)
> + !old_plane_state->fb != !fb)
> goto slow;
>
> new_plane_state = intel_plane_duplicate_state(plane);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ec16f3d6dd2e..660990a3f276 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1865,20 +1865,26 @@ 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;
> +
> /*
> - * 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.
> + * Treat cursor with fb as always visible since
> + * cursor updates can happen faster than the vrefresh
> + * rate, and the current watermark code doesn't handle
> + * that correctly. Cursor updates which set/clear the
> + * fb are going to get throttled by
> + * intel_legacy_cursor_update() to work around this
> + * problem with the watermark code.
> */
> - int cpp = 4;
> - int width = pstate->base.visible ? pstate->base.crtc_w : 64;
> -
> - if (!cstate->base.active)
> + if (!cstate->base.active || !pstate->base.fb)
> return 0;
>
> + cpp = pstate->base.fb->format->cpp[0];
> +
> return ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> cstate->base.adjusted_mode.crtc_htotal,
> - width, cpp, mem_value);
> + pstate->base.crtc_w,
> + cpp, mem_value);
> }
>
> /* Only for WM_LP. */
>
> and fix up the legacy_cursor_update flag problem with the slow path...
Would it be sensible to test this patch on my hardware? You seem to
think it's incomplete ...?!
I can already predict that several more people will be affected by this
when Debian Stretch is released (currently in freeze) and people start
upgrading to this.
Best regards
Uwe
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
