Re: [Intel-gfx] [PATCH] drm/i915: do not reset PLANE_SURF on plane disable on older gens

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 05.09.2022 13:48, Ville Syrjälä wrote:
> On Mon, Sep 05, 2022 at 10:05:00AM +0200, Andrzej Hajda wrote:
>> In case of ICL and older generations disabling plane and/or disabling
>> async update is always performed on vblank,
> It should only be broken on bdw-glk (see. need_async_flip_disable_wa).

On CFL it is reported every drmtip run:
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip.html?testfilter=tiled-max-hw
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1209/fi-cfl-8109u/igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html#dmesg-warnings402
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1209/fi-cfl-8109u/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html#dmesg-warnings402
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1209/fi-cfl-8109u/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1208/fi-cfl-8109u/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html
...
On APL it is less frequent, probably due to other bugs preventing run of 
this test, last seen at:
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1190/fi-apl-guc/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html
Similar for SKL:
https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_1181/fi-skl-guc/igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html

I am not sure if I correctly read the docs but [1] says that 9th bit of 
PLANE_CFG (Async Address Update Enable) is "not double buffered and the 
changes will apply immediately" only for ICL, JSL, LKF1.
So the change is not necessary in case of icl_plane_disable_arm.

[1]: https://gfxspecs.intel.com/Predator/Home/Index/7656
>
>> but if async update is enabled
>> PLANE_SURF register is updated asynchronously. Writing 0 to PLANE_SURF
>> when plane is still enabled can cause DMAR/PIPE errors.
>> On the other side PLANE_SURF is used to arm plane registers - we need to
>> write to it to trigger update on VBLANK, writting current value should
>> be safe - the buffer address is valid till vblank.
> I think you're effectively saying that somehow the async
> flip disable w/a is not kicking in sometimes.

I was not aware of existence of this w/a and I am little lost in 
figuring out how this w/a can prevent zeroing PLANE_SURF too early.

Regards
Andrzej

>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/skl_universal_plane.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index bcfde81e4d0866..bc9ed60a2d349e 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -615,11 +615,13 @@ skl_plane_disable_arm(struct intel_plane *plane,
>>   	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>   	enum plane_id plane_id = plane->id;
>>   	enum pipe pipe = plane->pipe;
>> +	u32 plane_surf;
>>   
>>   	skl_write_plane_wm(plane, crtc_state);
>>   
>>   	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), 0);
>> -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), 0);
>> +	plane_surf = intel_de_read_fw(dev_priv, PLANE_SURF(pipe, plane_id));
>> +	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), plane_surf);
>>   }
>>   
>>   static void
>> @@ -629,6 +631,7 @@ icl_plane_disable_arm(struct intel_plane *plane,
>>   	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>   	enum plane_id plane_id = plane->id;
>>   	enum pipe pipe = plane->pipe;
>> +	u32 plane_surf;
>>   
>>   	if (icl_is_hdr_plane(dev_priv, plane_id))
>>   		intel_de_write_fw(dev_priv, PLANE_CUS_CTL(pipe, plane_id), 0);
>> @@ -637,7 +640,8 @@ icl_plane_disable_arm(struct intel_plane *plane,
>>   
>>   	intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
>>   	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), 0);
>> -	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), 0);
>> +	plane_surf = intel_de_read_fw(dev_priv, PLANE_SURF(pipe, plane_id));
>> +	intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), plane_surf);
>>   }
>>   
>>   static bool
>> -- 
>> 2.25.1