diff mbox series

[v2,1/6] drm/i915/display/psr: Calculate selective fetch plane registers

Message ID 20201027234526.33043-1-jose.souza@intel.com
State New, archived
Headers show
Series [v2,1/6] drm/i915/display/psr: Calculate selective fetch plane registers | expand

Commit Message

Souza, Jose Oct. 27, 2020, 11:45 p.m. UTC
Add the calculations to set plane selective fetch registers depending
in the value of the area damaged.
It is still using the whole plane area as damaged but that will change
in next patches.

v2:
- fixed new_plane_state->uapi.dst.y2 typo in
intel_psr2_sel_fetch_update()
- do not shifthing new_plane_state->uapi.dst only src is in 16.16 format

BSpec: 55229
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 .../drm/i915/display/intel_display_types.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

Comments

Souza, Jose Oct. 29, 2020, 12:13 a.m. UTC | #1
On Wed, 2020-10-28 at 05:54 +0000, Patchwork wrote:
Patch Details
Series: series starting with [v2,1/6] drm/i915/display/psr: Calculate selective fetch plane registers
URL:    https://patchwork.freedesktop.org/series/83119/
State:  failure
Details:        https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/index.html
CI Bug Log - changes from CI_DRM_9206 -> Patchwork_18793
Summary

FAILURE

Serious unknown changes coming with Patchwork_18793 absolutely need to be
verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_18793, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.

External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/index.html

Possible new issues

Here are the unknown changes that may have been introduced in Patchwork_18793:

IGT changes
Possible regressions

  *   igt@i915_selftest@live@gt_lrc:

     *   fi-bsw-n3050: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-bsw-n3050/igt@i915_selftest@live@gt_lrc.html> -> DMESG-FAIL<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-bsw-n3050/igt@i915_selftest@live@gt_lrc.html>

Unrelated.

  *   igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:

     *   fi-tgl-y: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html> -> FAIL<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html>

This one will be a test that we need to fix, it is using legacy api that do not calls current PSR2 sel fetch functions.
But for the initial enabling of selective fetch when doing page flips it is out of scope.

Warnings

  *   igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:

     *   fi-tgl-y: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982>) -> FAIL<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html>
  *   igt@runner@aborted:

     *   fi-cfl-8109u: FAIL<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-cfl-8109u/igt@runner@aborted.html> (k.org#202107<https://bugzilla.kernel.org/show_bug.cgi?id=202107> / k.org#202109<https://bugzilla.kernel.org/show_bug.cgi?id=202109>) -> FAIL<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-cfl-8109u/igt@runner@aborted.html>

New tests

New tests have been introduced between CI_DRM_9206 and Patchwork_18793:

New CI tests (1)

  *   boot:
     *   Statuses : 41 pass(s)
     *   Exec time: [0.0] s

Known issues

Here are the changes found in Patchwork_18793 that come from known issues:

IGT changes
Issues hit

  *   igt@debugfs_test@read_all_entries:

     *   fi-tgl-u2: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-u2/igt@debugfs_test@read_all_entries.html> -> DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-u2/igt@debugfs_test@read_all_entries.html> (i915#402<https://gitlab.freedesktop.org/drm/intel/issues/402>)
  *   igt@gem_ctx_create@basic-files:

     *   fi-apl-guc: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-apl-guc/igt@gem_ctx_create@basic-files.html> -> INCOMPLETE<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-apl-guc/igt@gem_ctx_create@basic-files.html> (i915#1635<https://gitlab.freedesktop.org/drm/intel/issues/1635>)
  *   igt@i915_module_load@reload:

     *   fi-byt-j1900: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-byt-j1900/igt@i915_module_load@reload.html> -> DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-byt-j1900/igt@i915_module_load@reload.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982>)
  *   igt@kms_busy@basic@flip:

     *   fi-kbl-soraka: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-kbl-soraka/igt@kms_busy@basic@flip.html> -> DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-kbl-soraka/igt@kms_busy@basic@flip.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982>)

     *   fi-tgl-y: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-y/igt@kms_busy@basic@flip.html> -> DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-y/igt@kms_busy@basic@flip.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982>) +1 similar issue

  *   igt@kms_chamelium@dp-crc-fast:

     *   fi-cml-u2: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html> -> INCOMPLETE<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html> (i915#2606<https://gitlab.freedesktop.org/drm/intel/issues/2606>)
  *   igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:

     *   fi-tgl-y: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-y/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html> -> DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-y/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html> (i915#402<https://gitlab.freedesktop.org/drm/intel/issues/402>) +5 similar issues
  *   igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:

     *   fi-tgl-y: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-y/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html> -> DMESG-FAIL<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-y/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html> (i915#402<https://gitlab.freedesktop.org/drm/intel/issues/402>) +1 similar issue
  *   igt@kms_psr@primary_page_flip:

     *   fi-tgl-u2: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-u2/igt@kms_psr@primary_page_flip.html> -> SKIP<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-u2/igt@kms_psr@primary_page_flip.html> (i915#668<https://gitlab.freedesktop.org/drm/intel/issues/668>) +1 similar issue

     *   fi-kbl-soraka: PASS<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-kbl-soraka/igt@kms_psr@primary_page_flip.html> -> INCOMPLETE<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-kbl-soraka/igt@kms_psr@primary_page_flip.html> (i915#2606<https://gitlab.freedesktop.org/drm/intel/issues/2606>)

Possible fixes

  *   igt@core_hotunplug@unbind-rebind:

     *   fi-blb-e6850: INCOMPLETE<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-blb-e6850/igt@core_hotunplug@unbind-rebind.html> (i915#2540<https://gitlab.freedesktop.org/drm/intel/issues/2540>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-blb-e6850/igt@core_hotunplug@unbind-rebind.html>
  *   igt@debugfs_test@read_all_entries:

     *   {fi-kbl-7560u}: INCOMPLETE<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-kbl-7560u/igt@debugfs_test@read_all_entries.html> (i915#2417<https://gitlab.freedesktop.org/drm/intel/issues/2417>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-kbl-7560u/igt@debugfs_test@read_all_entries.html>
  *   igt@gem_exec_suspend@basic-s3:

     *   fi-byt-j1900: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-byt-j1900/igt@gem_exec_suspend@basic-s3.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-byt-j1900/igt@gem_exec_suspend@basic-s3.html>
  *   igt@i915_module_load@reload:

     *   {fi-ehl-1}: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-ehl-1/igt@i915_module_load@reload.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-ehl-1/igt@i915_module_load@reload.html>

     *   fi-skl-lmem: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-skl-lmem/igt@i915_module_load@reload.html> (i915#2605<https://gitlab.freedesktop.org/drm/intel/issues/2605>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-skl-lmem/igt@i915_module_load@reload.html>

  *   igt@i915_pm_rpm@basic-pci-d3-state:

     *   fi-bsw-kefka: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html>
  *   igt@kms_psr@sprite_plane_onoff:

     *   fi-tgl-y: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-y/igt@kms_psr@sprite_plane_onoff.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-y/igt@kms_psr@sprite_plane_onoff.html>
  *   igt@prime_vgem@basic-read:

     *   fi-tgl-y: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-y/igt@prime_vgem@basic-read.html> (i915#402<https://gitlab.freedesktop.org/drm/intel/issues/402>) -> PASS<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-y/igt@prime_vgem@basic-read.html> +1 similar issue

Warnings

  *   igt@i915_pm_rpm@module-reload:
     *   fi-tgl-y: DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9206/fi-tgl-y/igt@i915_pm_rpm@module-reload.html> (i915#2411<https://gitlab.freedesktop.org/drm/intel/issues/2411>) -> DMESG-WARN<https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18793/fi-tgl-y/igt@i915_pm_rpm@module-reload.html> (i915#1982<https://gitlab.freedesktop.org/drm/intel/issues/1982> / i915#2411<https://gitlab.freedesktop.org/drm/intel/issues/2411>)

{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).

Participating hosts (43 -> 41)

Missing (2): fi-bsw-cyan fi-bdw-samus

Build changes

  *   Linux: CI_DRM_9206 -> Patchwork_18793

CI-20190529: 20190529
CI_DRM_9206: 85ce674ff932ed7ca41aef52d8bb42c04fbe2171 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5827: 7fd7e3fb8b42eb4e62a4575f6edc5a048e5bec3d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_18793: 524e59edd723db54e5dcc3282bed18a6b08d5b66 @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

524e59edd723 HAX/DO_NOT_MERGE_IT: drm/i915/display: Enable PSR2 selective fetch for testing
4bb764e0fd8c RFC/WIP: drm/i915/display/psr: Consider tiling when doing the plane offset calculation
a542d2e2472d drm/i915/display: Split and export main surface calculation from skl_check_main_surface()
8fc4928b3976 drm/i915/display/psr: Consider other planes to damaged area calculation
ed22efb12033 drm/i915/display/psr: Use plane damage clips to calculate damaged area
0a61bec54546 drm/i915/display/psr: Calculate selective fetch plane registers
Gwan-gyeong Mun Oct. 29, 2020, 9:37 p.m. UTC | #2
On Tue, 2020-10-27 at 16:45 -0700, José Roberto de Souza wrote:
> Add the calculations to set plane selective fetch registers depending
> in the value of the area damaged.
> It is still using the whole plane area as damaged but that will
> change
> in next patches.
> 
> v2:
> - fixed new_plane_state->uapi.dst.y2 typo in
> intel_psr2_sel_fetch_update()
> - do not shifthing new_plane_state->uapi.dst only src is in 16.16
> format
> 
> BSpec: 55229
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 ++
>  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++---
> --
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index f6f0626649e0..3f2707d882cc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -604,6 +604,8 @@ struct intel_plane_state {
>  	u32 planar_slave;
>  
>  	struct drm_intel_sprite_colorkey ckey;
> +
> +	struct drm_rect psr2_sel_fetch_area;
>  };
>  
>  struct intel_initial_plane_config {
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index b2544102e7b1..6dead51d7a81 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1187,6 +1187,7 @@ void intel_psr2_program_plane_sel_fetch(struct
> intel_plane *plane,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum pipe pipe = plane->pipe;
> +	const struct drm_rect *clip;
>  	u32 val;
>  
>  	if (!crtc_state->enable_psr2_sel_fetch)
> @@ -1198,16 +1199,20 @@ void
> intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
>  	if (!val || plane->id == PLANE_CURSOR)
>  		return;
>  
> -	val = plane_state->uapi.dst.y1 << 16 | plane_state-
> >uapi.dst.x1;
> +	clip = &plane_state->psr2_sel_fetch_area;
> +
> +	val = (clip->y1 + plane_state->uapi.dst.y1) << 16;
> +	val |= plane_state->uapi.dst.x1;
>  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane-
> >id), val);
>  
> -	val = plane_state->color_plane[color_plane].y << 16;
> +	/* TODO: consider tiling and auxiliary surfaces */
> +	val = (clip->y1 + plane_state->color_plane[color_plane].y) <<
> 16;
>  	val |= plane_state->color_plane[color_plane].x;
>  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane-
> >id),
>  			  val);
>  
>  	/* Sizes are 0 based */
> -	val = ((drm_rect_height(&plane_state->uapi.src) >> 16) - 1) <<
> 16;
> +	val = (drm_rect_height(clip) - 1) << 16;
>  	val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
>  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane-
> >id), val);
>  }
> @@ -1281,7 +1286,7 @@ int intel_psr2_sel_fetch_update(struct
> intel_atomic_state *state,
>  
>  	for_each_oldnew_intel_plane_in_state(state, plane,
> old_plane_state,
>  					     new_plane_state, i) {
> -		struct drm_rect temp;
> +		struct drm_rect *sel_fetch_area, temp;
>  
>  		if (new_plane_state->uapi.crtc != crtc_state-
> >uapi.crtc)
>  			continue;
> @@ -1304,8 +1309,13 @@ int intel_psr2_sel_fetch_update(struct
> intel_atomic_state *state,
>  		 * For now doing a selective fetch in the whole plane
> area,
>  		 * optimizations will come in the future.
>  		 */
> -		temp.y1 = new_plane_state->uapi.dst.y1;
> -		temp.y2 = new_plane_state->uapi.dst.y2;
> +		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> +		sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >>
> 16;
> +		sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >>
> 16;
> +
> +		temp = *sel_fetch_area;
> +		temp.y1 += new_plane_state->uapi.dst.y1;
> +		temp.y2 += new_plane_state->uapi.dst.y2;
when the userspace call drmModeSetPlane(), src_y and crtc_y can be
differ.
(drm core uses drm_rect_translate_to() utility function for apply
coordinate traslate to dst from src. )

In my opinion, we can calculate like as,
int coord_trans = new_plane_state->uapi.dst.y1 - new_plane_state-
>uapi.src.y1 >> 16;
temp = *sel_fetch_area; // assunes that sel_fetch_area has damage area.
temp.y1 += coord_trans;
temp.y2 += coord_trans;

>  		clip_area_update(&pipe_clip, &temp);

As we don't check and calculate plane rotation and scale for dst
coordinates here.
so can you add checking of plane scale or rotation? and if the plane
has a scale factor or rotation, for now, we should not apply PSR SU.
(if plane scale or rotation are used, dst might differ. if we don't
apply PSR SU, we don't need to call clip_area_update() here. )
>  	}
>
Souza, Jose Nov. 2, 2020, 7:18 p.m. UTC | #3
On Thu, 2020-10-29 at 21:37 +0000, Mun, Gwan-gyeong wrote:
> On Tue, 2020-10-27 at 16:45 -0700, José Roberto de Souza wrote:
> > Add the calculations to set plane selective fetch registers depending
> > in the value of the area damaged.
> > It is still using the whole plane area as damaged but that will
> > change
> > in next patches.
> > 
> > v2:
> > - fixed new_plane_state->uapi.dst.y2 typo in
> > intel_psr2_sel_fetch_update()
> > - do not shifthing new_plane_state->uapi.dst only src is in 16.16
> > format
> > 
> > BSpec: 55229
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 ++
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++---
> > --
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index f6f0626649e0..3f2707d882cc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -604,6 +604,8 @@ struct intel_plane_state {
> >  	u32 planar_slave;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  	struct drm_intel_sprite_colorkey ckey;
> > +
> > +	struct drm_rect psr2_sel_fetch_area;
> >  };
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  struct intel_initial_plane_config {
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index b2544102e7b1..6dead51d7a81 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1187,6 +1187,7 @@ void intel_psr2_program_plane_sel_fetch(struct
> > intel_plane *plane,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	enum pipe pipe = plane->pipe;
> > +	const struct drm_rect *clip;
> >  	u32 val;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  	if (!crtc_state->enable_psr2_sel_fetch)
> > @@ -1198,16 +1199,20 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> >  	if (!val || plane->id == PLANE_CURSOR)
> >  		return;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > -	val = plane_state->uapi.dst.y1 << 16 | plane_state-
> > > uapi.dst.x1;
> > +	clip = &plane_state->psr2_sel_fetch_area;
> > +
> > +	val = (clip->y1 + plane_state->uapi.dst.y1) << 16;
> > +	val |= plane_state->uapi.dst.x1;
> >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane-
> > > id), val);
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > -	val = plane_state->color_plane[color_plane].y << 16;
> > +	/* TODO: consider tiling and auxiliary surfaces */
> > +	val = (clip->y1 + plane_state->color_plane[color_plane].y) <<
> > 16;
> >  	val |= plane_state->color_plane[color_plane].x;
> >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane-
> > > id),
> >  			  val);
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  	/* Sizes are 0 based */
> > -	val = ((drm_rect_height(&plane_state->uapi.src) >> 16) - 1) <<
> > 16;
> > +	val = (drm_rect_height(clip) - 1) << 16;
> >  	val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
> >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane-
> > > id), val);
> >  }
> > @@ -1281,7 +1286,7 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  	for_each_oldnew_intel_plane_in_state(state, plane,
> > old_plane_state,
> >  					     new_plane_state, i) {
> > -		struct drm_rect temp;
> > +		struct drm_rect *sel_fetch_area, temp;
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > uapi.crtc)
> >  			continue;
> > @@ -1304,8 +1309,13 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  		 * For now doing a selective fetch in the whole plane
> > area,
> >  		 * optimizations will come in the future.
> >  		 */
> > -		temp.y1 = new_plane_state->uapi.dst.y1;
> > -		temp.y2 = new_plane_state->uapi.dst.y2;
> > +		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> > +		sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >>
> > 16;
> > +		sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >>
> > 16;
> > +
> > +		temp = *sel_fetch_area;
> > +		temp.y1 += new_plane_state->uapi.dst.y1;
> > +		temp.y2 += new_plane_state->uapi.dst.y2;
> when the userspace call drmModeSetPlane(), src_y and crtc_y can be
> differ.
> (drm core uses drm_rect_translate_to() utility function for apply
> coordinate traslate to dst from src. )

Checking drm_atomic_helper_update_plane() it sets userspace values to plane_state->crtc_ and plane_state->src_, then atomic code will set the right
values back to uapi.dst and uapi.src.

> 
> In my opinion, we can calculate like as,
> int coord_trans = new_plane_state->uapi.dst.y1 - new_plane_state-
> > uapi.src.y1 >> 16;
> temp = *sel_fetch_area; // assunes that sel_fetch_area has damage area.
> temp.y1 += coord_trans;
> temp.y2 += coord_trans;

uapi.dst.x1/y1 is the coordinates where plane should be placed in pipe to be combined with other planes(skl_program_plane()), that exactly what we
need to compute the damaged areas of the whole pipe.

> 
> >  		clip_area_update(&pipe_clip, &temp);
> 
> As we don't check and calculate plane rotation and scale for dst
> coordinates here.
> so can you add checking of plane scale or rotation? and if the plane
> has a scale factor or rotation, for now, we should not apply PSR SU.
> (if plane scale or rotation are used, dst might differ. if we don't
> apply PSR SU, we don't need to call clip_area_update() here. )

Rotation is not supported by selective fetch(intel_psr2_sel_fetch_config_valid()) about scale, the scaler HW will take care of adjust the original
size but I doubt that scale works with PSR panels.

> >  	}
> >  
> > 
> > 
> >
Gwan-gyeong Mun Nov. 27, 2020, 10:13 a.m. UTC | #4
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Tested-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>

On Mon, 2020-11-02 at 11:18 -0800, Souza, Jose wrote:
> On Thu, 2020-10-29 at 21:37 +0000, Mun, Gwan-gyeong wrote:
> > On Tue, 2020-10-27 at 16:45 -0700, José Roberto de Souza wrote:
> > > Add the calculations to set plane selective fetch registers
> > > depending
> > > in the value of the area damaged.
> > > It is still using the whole plane area as damaged but that will
> > > change
> > > in next patches.
> > > 
> > > v2:
> > > - fixed new_plane_state->uapi.dst.y2 typo in
> > > intel_psr2_sel_fetch_update()
> > > - do not shifthing new_plane_state->uapi.dst only src is in 16.16
> > > format
> > > 
> > > BSpec: 55229
> > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_types.h    |  2 ++
> > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22
> > > ++++++++++++++---
> > > --
> > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index f6f0626649e0..3f2707d882cc 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -604,6 +604,8 @@ struct intel_plane_state {
> > >  	u32 planar_slave;
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  	struct drm_intel_sprite_colorkey ckey;
> > > +
> > > +	struct drm_rect psr2_sel_fetch_area;
> > >  };
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  struct intel_initial_plane_config {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index b2544102e7b1..6dead51d7a81 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1187,6 +1187,7 @@ void
> > > intel_psr2_program_plane_sel_fetch(struct
> > > intel_plane *plane,
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > >  	enum pipe pipe = plane->pipe;
> > > +	const struct drm_rect *clip;
> > >  	u32 val;
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  	if (!crtc_state->enable_psr2_sel_fetch)
> > > @@ -1198,16 +1199,20 @@ void
> > > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > >  	if (!val || plane->id == PLANE_CURSOR)
> > >  		return;
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > -	val = plane_state->uapi.dst.y1 << 16 | plane_state-
> > > > uapi.dst.x1;
> > > +	clip = &plane_state->psr2_sel_fetch_area;
> > > +
> > > +	val = (clip->y1 + plane_state->uapi.dst.y1) << 16;
> > > +	val |= plane_state->uapi.dst.x1;
> > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane-
> > > > id), val);
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > -	val = plane_state->color_plane[color_plane].y << 16;
> > > +	/* TODO: consider tiling and auxiliary surfaces */
> > > +	val = (clip->y1 + plane_state->color_plane[color_plane].y) <<
> > > 16;
> > >  	val |= plane_state->color_plane[color_plane].x;
> > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane-
> > > > id),
> > >  			  val);
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  	/* Sizes are 0 based */
> > > -	val = ((drm_rect_height(&plane_state->uapi.src) >> 16) - 1) <<
> > > 16;
> > > +	val = (drm_rect_height(clip) - 1) << 16;
> > >  	val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
> > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane-
> > > > id), val);
> > >  }
> > > @@ -1281,7 +1286,7 @@ int intel_psr2_sel_fetch_update(struct
> > > intel_atomic_state *state,
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  	for_each_oldnew_intel_plane_in_state(state, plane,
> > > old_plane_state,
> > >  					     new_plane_state, i) {
> > > -		struct drm_rect temp;
> > > +		struct drm_rect *sel_fetch_area, temp;
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  		if (new_plane_state->uapi.crtc != crtc_state-
> > > > uapi.crtc)
> > >  			continue;
> > > @@ -1304,8 +1309,13 @@ int intel_psr2_sel_fetch_update(struct
> > > intel_atomic_state *state,
> > >  		 * For now doing a selective fetch in the whole plane
> > > area,
> > >  		 * optimizations will come in the future.
> > >  		 */
> > > -		temp.y1 = new_plane_state->uapi.dst.y1;
> > > -		temp.y2 = new_plane_state->uapi.dst.y2;
> > > +		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
> > > +		sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >>
> > > 16;
> > > +		sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >>
> > > 16;
> > > +
> > > +		temp = *sel_fetch_area;
> > > +		temp.y1 += new_plane_state->uapi.dst.y1;
> > > +		temp.y2 += new_plane_state->uapi.dst.y2;
> > when the userspace call drmModeSetPlane(), src_y and crtc_y can be
> > differ.
> > (drm core uses drm_rect_translate_to() utility function for apply
> > coordinate traslate to dst from src. )
> 
> Checking drm_atomic_helper_update_plane() it sets userspace values to
> plane_state->crtc_ and plane_state->src_, then atomic code will set
> the right
> values back to uapi.dst and uapi.src.
> 
> > In my opinion, we can calculate like as,
> > int coord_trans = new_plane_state->uapi.dst.y1 - new_plane_state-
> > > uapi.src.y1 >> 16;
> > temp = *sel_fetch_area; // assunes that sel_fetch_area has damage
> > area.
> > temp.y1 += coord_trans;
> > temp.y2 += coord_trans;
> 
> uapi.dst.x1/y1 is the coordinates where plane should be placed in
> pipe to be combined with other planes(skl_program_plane()), that
> exactly what we
> need to compute the damaged areas of the whole pipe.
> 
> > >  		clip_area_update(&pipe_clip, &temp);
> > 
> > As we don't check and calculate plane rotation and scale for dst
> > coordinates here.
> > so can you add checking of plane scale or rotation? and if the
> > plane
> > has a scale factor or rotation, for now, we should not apply PSR
> > SU.
> > (if plane scale or rotation are used, dst might differ. if we don't
> > apply PSR SU, we don't need to call clip_area_update() here. )
> 
> Rotation is not supported by selective
> fetch(intel_psr2_sel_fetch_config_valid()) about scale, the scaler HW
> will take care of adjust the original
> size but I doubt that scale works with PSR panels.
> 
> > >  	}
> > >  
> > > 
> > > 
> > >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index f6f0626649e0..3f2707d882cc 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -604,6 +604,8 @@  struct intel_plane_state {
 	u32 planar_slave;
 
 	struct drm_intel_sprite_colorkey ckey;
+
+	struct drm_rect psr2_sel_fetch_area;
 };
 
 struct intel_initial_plane_config {
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index b2544102e7b1..6dead51d7a81 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1187,6 +1187,7 @@  void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
+	const struct drm_rect *clip;
 	u32 val;
 
 	if (!crtc_state->enable_psr2_sel_fetch)
@@ -1198,16 +1199,20 @@  void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
 	if (!val || plane->id == PLANE_CURSOR)
 		return;
 
-	val = plane_state->uapi.dst.y1 << 16 | plane_state->uapi.dst.x1;
+	clip = &plane_state->psr2_sel_fetch_area;
+
+	val = (clip->y1 + plane_state->uapi.dst.y1) << 16;
+	val |= plane_state->uapi.dst.x1;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_POS(pipe, plane->id), val);
 
-	val = plane_state->color_plane[color_plane].y << 16;
+	/* TODO: consider tiling and auxiliary surfaces */
+	val = (clip->y1 + plane_state->color_plane[color_plane].y) << 16;
 	val |= plane_state->color_plane[color_plane].x;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_OFFSET(pipe, plane->id),
 			  val);
 
 	/* Sizes are 0 based */
-	val = ((drm_rect_height(&plane_state->uapi.src) >> 16) - 1) << 16;
+	val = (drm_rect_height(clip) - 1) << 16;
 	val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
 	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane->id), val);
 }
@@ -1281,7 +1286,7 @@  int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 
 	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
 					     new_plane_state, i) {
-		struct drm_rect temp;
+		struct drm_rect *sel_fetch_area, temp;
 
 		if (new_plane_state->uapi.crtc != crtc_state->uapi.crtc)
 			continue;
@@ -1304,8 +1309,13 @@  int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
 		 * For now doing a selective fetch in the whole plane area,
 		 * optimizations will come in the future.
 		 */
-		temp.y1 = new_plane_state->uapi.dst.y1;
-		temp.y2 = new_plane_state->uapi.dst.y2;
+		sel_fetch_area = &new_plane_state->psr2_sel_fetch_area;
+		sel_fetch_area->y1 = new_plane_state->uapi.src.y1 >> 16;
+		sel_fetch_area->y2 = new_plane_state->uapi.src.y2 >> 16;
+
+		temp = *sel_fetch_area;
+		temp.y1 += new_plane_state->uapi.dst.y1;
+		temp.y2 += new_plane_state->uapi.dst.y2;
 		clip_area_update(&pipe_clip, &temp);
 	}