diff mbox series

[1/2] drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color

Message ID 20201123182631.1740781-1-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color | expand

Commit Message

Imre Deak Nov. 23, 2020, 6:26 p.m. UTC
From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>

Gen12 display can decompress surfaces compressed by render engine with
Clear Color, add a new modifier as the driver needs to know the surface
was compressed by render engine.

V2: Description changes as suggested by Rafael.
V3: Mention the Clear Color size of 64 bits in the comments(DK)
v4: Fix trailing whitespaces
v5: Explain Clear Color in the documentation.
v6: Documentation Nitpicks(Nanley)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Nanley Chery <nanley.g.chery@intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Imre Deak Nov. 24, 2020, 3:12 p.m. UTC | #1
Hi,

On Mon, Nov 23, 2020 at 09:24:31PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/2] drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color
> URL   : https://patchwork.freedesktop.org/series/84183/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_9378 -> Patchwork_18961
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_18961 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_18961, 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_18961/index.html
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in Patchwork_18961:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@gem_exec_fence@basic-await@rcs0:
>     - fi-skl-lmem:        [PASS][1] -> [INCOMPLETE][2]
>    [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-skl-lmem/igt@gem_exec_fence@basic-await@rcs0.html
>    [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-skl-lmem/igt@gem_exec_fence@basic-await@rcs0.html
> 
>   * igt@kms_chamelium@common-hpd-after-suspend:
>     - fi-kbl-7500u:       [PASS][3] -> [FAIL][4]
>    [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html
>    [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html

the patchset adds a new framebuffer modifier used only on TGL, so the
above two failures happen on unaffected platforms.

> 
>   
> New tests
> ---------
> 
>   New tests have been introduced between CI_DRM_9378 and Patchwork_18961:
> 
> ### New CI tests (1) ###
> 
>   * boot:
>     - Statuses : 40 pass(s)
>     - Exec time: [0.0] s
> 
>   
> 
> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_18961 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@kms_busy@basic@flip:
>     - fi-tgl-y:           [PASS][5] -> [DMESG-WARN][6] ([i915#1982]) +1 similar issue
>    [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-tgl-y/igt@kms_busy@basic@flip.html
>    [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-tgl-y/igt@kms_busy@basic@flip.html
> 
>   * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
>     - fi-icl-u2:          [PASS][7] -> [DMESG-WARN][8] ([i915#1982]) +1 similar issue
>    [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
>    [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
> 
>   * igt@prime_self_import@basic-with_one_bo_two_files:
>     - fi-tgl-y:           [PASS][9] -> [DMESG-WARN][10] ([i915#402]) +1 similar issue
>    [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html
>    [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html
> 
>   
> #### Possible fixes ####
> 
>   * igt@kms_busy@basic@modeset:
>     - fi-tgl-y:           [DMESG-WARN][11] ([i915#1982]) -> [PASS][12] +1 similar issue
>    [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-tgl-y/igt@kms_busy@basic@modeset.html
>    [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-tgl-y/igt@kms_busy@basic@modeset.html
> 
>   * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
>     - fi-byt-j1900:       [DMESG-WARN][13] ([i915#1982]) -> [PASS][14] +1 similar issue
>    [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-byt-j1900/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
>    [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-byt-j1900/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
>     - {fi-kbl-7560u}:     [DMESG-WARN][15] ([i915#1982]) -> [PASS][16]
>    [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
>    [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
> 
>   * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
>     - fi-icl-u2:          [DMESG-WARN][17] ([i915#1982]) -> [PASS][18]
>    [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
>    [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
> 
>   * igt@prime_vgem@basic-write:
>     - fi-tgl-y:           [DMESG-WARN][19] ([i915#402]) -> [PASS][20] +1 similar issue
>    [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-tgl-y/igt@prime_vgem@basic-write.html
>    [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-tgl-y/igt@prime_vgem@basic-write.html
> 
>   
>   {name}: This element is suppressed. This means it is ignored when computing
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>   [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
>   [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
> 
> 
> Participating hosts (44 -> 40)
> ------------------------------
> 
>   Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 
> 
> 
> Build changes
> -------------
> 
>   * Linux: CI_DRM_9378 -> Patchwork_18961
> 
>   CI-20190529: 20190529
>   CI_DRM_9378: efc7f880143d6fe75922ad393045665c8ea60f57 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_5868: 36b5fc05c30dbfd9242069fd6e51ebb419b386bc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_18961: 0dba847db9d24c6d2d706379ac2f842b8f4a6bdb @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> 0dba847db9d2 drm/i915/tgl: Add Clear Color support for TGL Render Decompression
> a3b4d1336125 drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/index.html
Vudum, Lakshminarayana Nov. 24, 2020, 5:15 p.m. UTC | #2
Re-reported.

-----Original Message-----
From: Imre Deak <imre.deak@intel.com> 
Sent: Tuesday, November 24, 2020 7:13 AM
To: intel-gfx@lists.freedesktop.org; Vudum, Lakshminarayana <lakshminarayana.vudum@intel.com>
Subject: Re: ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color

Hi,

On Mon, Nov 23, 2020 at 09:24:31PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/2] drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color
> URL   : https://patchwork.freedesktop.org/series/84183/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_9378 -> Patchwork_18961 
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_18961 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_18961, 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_18961/index.html
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in Patchwork_18961:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@gem_exec_fence@basic-await@rcs0:
>     - fi-skl-lmem:        [PASS][1] -> [INCOMPLETE][2]
>    [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-skl-lmem/igt@gem_exec_fence@basic-await@rcs0.html
>    [2]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-skl-lmem/i
> gt@gem_exec_fence@basic-await@rcs0.html
> 
>   * igt@kms_chamelium@common-hpd-after-suspend:
>     - fi-kbl-7500u:       [PASS][3] -> [FAIL][4]
>    [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html
>    [4]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-kbl-7500u/
> igt@kms_chamelium@common-hpd-after-suspend.html

the patchset adds a new framebuffer modifier used only on TGL, so the above two failures happen on unaffected platforms.

> 
>   
> New tests
> ---------
> 
>   New tests have been introduced between CI_DRM_9378 and Patchwork_18961:
> 
> ### New CI tests (1) ###
> 
>   * boot:
>     - Statuses : 40 pass(s)
>     - Exec time: [0.0] s
> 
>   
> 
> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_18961 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@kms_busy@basic@flip:
>     - fi-tgl-y:           [PASS][5] -> [DMESG-WARN][6] ([i915#1982]) +1 similar issue
>    [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-tgl-y/igt@kms_busy@basic@flip.html
>    [6]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-tgl-y/igt@
> kms_busy@basic@flip.html
> 
>   * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
>     - fi-icl-u2:          [PASS][7] -> [DMESG-WARN][8] ([i915#1982]) +1 similar issue
>    [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
>    [8]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-icl-u2/igt
> @kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
> 
>   * igt@prime_self_import@basic-with_one_bo_two_files:
>     - fi-tgl-y:           [PASS][9] -> [DMESG-WARN][10] ([i915#402]) +1 similar issue
>    [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html
>    [10]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-tgl-y/igt@
> prime_self_import@basic-with_one_bo_two_files.html
> 
>   
> #### Possible fixes ####
> 
>   * igt@kms_busy@basic@modeset:
>     - fi-tgl-y:           [DMESG-WARN][11] ([i915#1982]) -> [PASS][12] +1 similar issue
>    [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-tgl-y/igt@kms_busy@basic@modeset.html
>    [12]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-tgl-y/igt@
> kms_busy@basic@modeset.html
> 
>   * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
>     - fi-byt-j1900:       [DMESG-WARN][13] ([i915#1982]) -> [PASS][14] +1 similar issue
>    [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-byt-j1900/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
>    [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-byt-j1900/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
>     - {fi-kbl-7560u}:     [DMESG-WARN][15] ([i915#1982]) -> [PASS][16]
>    [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
>    [16]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-kbl-7560u/
> igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
> 
>   * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
>     - fi-icl-u2:          [DMESG-WARN][17] ([i915#1982]) -> [PASS][18]
>    [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
>    [18]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-icl-u2/igt
> @kms_cursor_legacy@basic-flip-after-cursor-legacy.html
> 
>   * igt@prime_vgem@basic-write:
>     - fi-tgl-y:           [DMESG-WARN][19] ([i915#402]) -> [PASS][20] +1 similar issue
>    [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9378/fi-tgl-y/igt@prime_vgem@basic-write.html
>    [20]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/fi-tgl-y/igt@
> prime_vgem@basic-write.html
> 
>   
>   {name}: This element is suppressed. This means it is ignored when computing
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>   [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
>   [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
> 
> 
> Participating hosts (44 -> 40)
> ------------------------------
> 
>   Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 
> 
> 
> Build changes
> -------------
> 
>   * Linux: CI_DRM_9378 -> Patchwork_18961
> 
>   CI-20190529: 20190529
>   CI_DRM_9378: efc7f880143d6fe75922ad393045665c8ea60f57 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_5868: 36b5fc05c30dbfd9242069fd6e51ebb419b386bc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_18961: 0dba847db9d24c6d2d706379ac2f842b8f4a6bdb @ 
> git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> 0dba847db9d2 drm/i915/tgl: Add Clear Color support for TGL Render 
> Decompression
> a3b4d1336125 drm/framebuffer: Format modifier for Intel Gen 12 render 
> compression with Clear Color
> 
> == Logs ==
> 
> For more details see: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18961/index.html
Kahola, Mika Nov. 26, 2020, 8:24 a.m. UTC | #3
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre
> Deak
> Sent: Monday, November 23, 2020 8:27 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Chery, Nanley G <nanley.g.chery@intel.com>; Rafael Antognolli
> <rafael.antognolli@intel.com>; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com>; Kondapally, Kalyan
> <kalyan.kondapally@intel.com>
> Subject: [Intel-gfx] [PATCH 1/2] drm/framebuffer: Format modifier for Intel
> Gen 12 render compression with Clear Color
> 
> From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> 
> Gen12 display can decompress surfaces compressed by render engine with
> Clear Color, add a new modifier as the driver needs to know the surface was
> compressed by render engine.
> 
> V2: Description changes as suggested by Rafael.
> V3: Mention the Clear Color size of 64 bits in the comments(DK)
> v4: Fix trailing whitespaces
> v5: Explain Clear Color in the documentation.
> v6: Documentation Nitpicks(Nanley)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: Nanley Chery <nanley.g.chery@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> ---
>  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h
> b/include/uapi/drm/drm_fourcc.h index ca48ed0e6bc1..0a1b2c4c4bee
> 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -527,6 +527,25 @@ extern "C" {
>   */
>  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS
> fourcc_mod_code(INTEL, 7)
> 
> +/*
> + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
> + * compression.
> + *
> + * The main surface is Y-tiled and is at plane index 0 whereas CCS is
> +linear
> + * and at index 1. The clear color is stored at index 2, and the pitch
> +should
> + * be ignored. The clear color structure is 256 bits. The first 128
> +bits
> + * represents Raw Clear Color Red, Green, Blue and Alpha color each
> +represented
> + * by 32 bits. The raw clear color is consumed by the 3d engine and
> +generates
> + * the converted clear color of size 64 bits. The first 32 bits store
> +the Lower
> + * Converted Clear Color value and the next 32 bits store the Higher
> +Converted
> + * Clear Color value when applicable. The Converted Clear Color values
> +are
> + * consumed by the DE. The last 64 bits are used to store Color Discard
> +Enable
> + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache
> +line
> + * corresponds to an area of 4x1 tiles in the main surface. The main
> +surface
> + * pitch is required to be a multiple of 4 tile widths.
> + */
> +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC
> fourcc_mod_code(INTEL,
> +8)
> +
>  /*
>   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>   *
> --
> 2.25.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Imre Deak Nov. 27, 2020, 2:31 p.m. UTC | #4
Hi Daniel, Jani,

is it ok to merge this patch along with 2/2 via the i915 tree?

--Imre

On Mon, Nov 23, 2020 at 08:26:30PM +0200, Imre Deak wrote:
> From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> 
> Gen12 display can decompress surfaces compressed by render engine with
> Clear Color, add a new modifier as the driver needs to know the surface
> was compressed by render engine.
> 
> V2: Description changes as suggested by Rafael.
> V3: Mention the Clear Color size of 64 bits in the comments(DK)
> v4: Fix trailing whitespaces
> v5: Explain Clear Color in the documentation.
> v6: Documentation Nitpicks(Nanley)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: Nanley Chery <nanley.g.chery@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index ca48ed0e6bc1..0a1b2c4c4bee 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -527,6 +527,25 @@ extern "C" {
>   */
>  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
>  
> +/*
> + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
> + * compression.
> + *
> + * The main surface is Y-tiled and is at plane index 0 whereas CCS is linear
> + * and at index 1. The clear color is stored at index 2, and the pitch should
> + * be ignored. The clear color structure is 256 bits. The first 128 bits
> + * represents Raw Clear Color Red, Green, Blue and Alpha color each represented
> + * by 32 bits. The raw clear color is consumed by the 3d engine and generates
> + * the converted clear color of size 64 bits. The first 32 bits store the Lower
> + * Converted Clear Color value and the next 32 bits store the Higher Converted
> + * Clear Color value when applicable. The Converted Clear Color values are
> + * consumed by the DE. The last 64 bits are used to store Color Discard Enable
> + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache line
> + * corresponds to an area of 4x1 tiles in the main surface. The main surface
> + * pitch is required to be a multiple of 4 tile widths.
> + */
> +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
> +
>  /*
>   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>   *
> -- 
> 2.25.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Nov. 27, 2020, 3:19 p.m. UTC | #5
On Fri, Nov 27, 2020 at 04:31:00PM +0200, Imre Deak wrote:
> Hi Daniel, Jani,
> 
> is it ok to merge this patch along with 2/2 via the i915 tree?

Ack from mesa (userspace in general, but mesa is kinda mandatory) is
missing I think. With that

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> --Imre
> 
> On Mon, Nov 23, 2020 at 08:26:30PM +0200, Imre Deak wrote:
> > From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > 
> > Gen12 display can decompress surfaces compressed by render engine with
> > Clear Color, add a new modifier as the driver needs to know the surface
> > was compressed by render engine.
> > 
> > V2: Description changes as suggested by Rafael.
> > V3: Mention the Clear Color size of 64 bits in the comments(DK)
> > v4: Fix trailing whitespaces
> > v5: Explain Clear Color in the documentation.
> > v6: Documentation Nitpicks(Nanley)
> > 
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
> > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > Cc: Nanley Chery <nanley.g.chery@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index ca48ed0e6bc1..0a1b2c4c4bee 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -527,6 +527,25 @@ extern "C" {
> >   */
> >  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
> >  
> > +/*
> > + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
> > + * compression.
> > + *
> > + * The main surface is Y-tiled and is at plane index 0 whereas CCS is linear
> > + * and at index 1. The clear color is stored at index 2, and the pitch should
> > + * be ignored. The clear color structure is 256 bits. The first 128 bits
> > + * represents Raw Clear Color Red, Green, Blue and Alpha color each represented
> > + * by 32 bits. The raw clear color is consumed by the 3d engine and generates
> > + * the converted clear color of size 64 bits. The first 32 bits store the Lower
> > + * Converted Clear Color value and the next 32 bits store the Higher Converted
> > + * Clear Color value when applicable. The Converted Clear Color values are
> > + * consumed by the DE. The last 64 bits are used to store Color Discard Enable
> > + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache line
> > + * corresponds to an area of 4x1 tiles in the main surface. The main surface
> > + * pitch is required to be a multiple of 4 tile widths.
> > + */
> > +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
> > +
> >  /*
> >   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
> >   *
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Imre Deak Nov. 27, 2020, 6:06 p.m. UTC | #6
On Fri, Nov 27, 2020 at 04:19:20PM +0100, Daniel Vetter wrote:
> On Fri, Nov 27, 2020 at 04:31:00PM +0200, Imre Deak wrote:
> > Hi Daniel, Jani,
> > 
> > is it ok to merge this patch along with 2/2 via the i915 tree?
> 
> Ack from mesa (userspace in general, but mesa is kinda mandatory) is
> missing I think. With that
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks.

Nanley, could you ACK the patchset if they look ok from Mesa's POV? It
works as expected at least with the igt/kms_ccs RC-CC subtest.

--Imre

> > On Mon, Nov 23, 2020 at 08:26:30PM +0200, Imre Deak wrote:
> > > From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > 
> > > Gen12 display can decompress surfaces compressed by render engine with
> > > Clear Color, add a new modifier as the driver needs to know the surface
> > > was compressed by render engine.
> > > 
> > > V2: Description changes as suggested by Rafael.
> > > V3: Mention the Clear Color size of 64 bits in the comments(DK)
> > > v4: Fix trailing whitespaces
> > > v5: Explain Clear Color in the documentation.
> > > v6: Documentation Nitpicks(Nanley)
> > > 
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
> > > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > > Cc: Nanley Chery <nanley.g.chery@intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > index ca48ed0e6bc1..0a1b2c4c4bee 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -527,6 +527,25 @@ extern "C" {
> > >   */
> > >  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
> > >  
> > > +/*
> > > + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
> > > + * compression.
> > > + *
> > > + * The main surface is Y-tiled and is at plane index 0 whereas CCS is linear
> > > + * and at index 1. The clear color is stored at index 2, and the pitch should
> > > + * be ignored. The clear color structure is 256 bits. The first 128 bits
> > > + * represents Raw Clear Color Red, Green, Blue and Alpha color each represented
> > > + * by 32 bits. The raw clear color is consumed by the 3d engine and generates
> > > + * the converted clear color of size 64 bits. The first 32 bits store the Lower
> > > + * Converted Clear Color value and the next 32 bits store the Higher Converted
> > > + * Clear Color value when applicable. The Converted Clear Color values are
> > > + * consumed by the DE. The last 64 bits are used to store Color Discard Enable
> > > + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache line
> > > + * corresponds to an area of 4x1 tiles in the main surface. The main surface
> > > + * pitch is required to be a multiple of 4 tile widths.
> > > + */
> > > +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
> > > +
> > >  /*
> > >   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
> > >   *
> > > -- 
> > > 2.25.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Jani Nikula Nov. 30, 2020, 10 a.m. UTC | #7
On Fri, 27 Nov 2020, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Nov 27, 2020 at 04:31:00PM +0200, Imre Deak wrote:
>> Hi Daniel, Jani,
>> 
>> is it ok to merge this patch along with 2/2 via the i915 tree?
>
> Ack from mesa (userspace in general, but mesa is kinda mandatory) is
> missing I think. With that
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

With the same conditions,

Acked-by: Jani Nikula <jani.nikula@intel.com>


>
>> 
>> --Imre
>> 
>> On Mon, Nov 23, 2020 at 08:26:30PM +0200, Imre Deak wrote:
>> > From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>> > 
>> > Gen12 display can decompress surfaces compressed by render engine with
>> > Clear Color, add a new modifier as the driver needs to know the surface
>> > was compressed by render engine.
>> > 
>> > V2: Description changes as suggested by Rafael.
>> > V3: Mention the Clear Color size of 64 bits in the comments(DK)
>> > v4: Fix trailing whitespaces
>> > v5: Explain Clear Color in the documentation.
>> > v6: Documentation Nitpicks(Nanley)
>> > 
>> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
>> > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
>> > Cc: Nanley Chery <nanley.g.chery@intel.com>
>> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
>> >  1 file changed, 19 insertions(+)
>> > 
>> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>> > index ca48ed0e6bc1..0a1b2c4c4bee 100644
>> > --- a/include/uapi/drm/drm_fourcc.h
>> > +++ b/include/uapi/drm/drm_fourcc.h
>> > @@ -527,6 +527,25 @@ extern "C" {
>> >   */
>> >  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
>> >  
>> > +/*
>> > + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
>> > + * compression.
>> > + *
>> > + * The main surface is Y-tiled and is at plane index 0 whereas CCS is linear
>> > + * and at index 1. The clear color is stored at index 2, and the pitch should
>> > + * be ignored. The clear color structure is 256 bits. The first 128 bits
>> > + * represents Raw Clear Color Red, Green, Blue and Alpha color each represented
>> > + * by 32 bits. The raw clear color is consumed by the 3d engine and generates
>> > + * the converted clear color of size 64 bits. The first 32 bits store the Lower
>> > + * Converted Clear Color value and the next 32 bits store the Higher Converted
>> > + * Clear Color value when applicable. The Converted Clear Color values are
>> > + * consumed by the DE. The last 64 bits are used to store Color Discard Enable
>> > + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache line
>> > + * corresponds to an area of 4x1 tiles in the main surface. The main surface
>> > + * pitch is required to be a multiple of 4 tile widths.
>> > + */
>> > +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
>> > +
>> >  /*
>> >   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>> >   *
>> > -- 
>> > 2.25.1
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Chery, Nanley G Dec. 1, 2020, 12:18 a.m. UTC | #8
> -----Original Message-----
> From: Imre Deak <imre.deak@intel.com>
> Sent: Friday, November 27, 2020 10:06 AM
> To: Daniel Vetter <daniel@ffwll.ch>; Chery, Nanley G
> <nanley.g.chery@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>; Daniel
> Vetter <daniel.vetter@ffwll.ch>; Rafael Antognolli
> <rafael.antognolli@intel.com>; Kondapally, Kalyan
> <kalyan.kondapally@intel.com>; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com>; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/framebuffer: Format modifier for Intel
> Gen 12 render compression with Clear Color
> 
> On Fri, Nov 27, 2020 at 04:19:20PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 27, 2020 at 04:31:00PM +0200, Imre Deak wrote:
> > > Hi Daniel, Jani,
> > >
> > > is it ok to merge this patch along with 2/2 via the i915 tree?
> >
> > Ack from mesa (userspace in general, but mesa is kinda mandatory) is
> > missing I think. With that
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks.
> 
> Nanley, could you ACK the patchset if they look ok from Mesa's POV? It
> works as expected at least with the igt/kms_ccs RC-CC subtest.
> 

Hi Imre,
 
I have a question and a couple comments:

Is the map of the clear color address creating a new synchronization point between the GPU and CPU? If so, I wonder how this will impact performance. There was some talk of asynchronously updating the clear color register a while back. 

We probably don't have to update the header, but we noticed in our testing that the clear color prefers an alignment greater than 64B. Unfortunately, I can't find any bspec note about this. As long as the buffer creators are aware though, I think we should be fine. I don't know if this is the best forum to bring it up, but I thought I'd share.

Seems like the upper converted clear color is untested due to the lack of RGBX16 support. I suppose that if there are any issues there, they can be fixed later...

-Nanley

> --Imre
> 
> > > On Mon, Nov 23, 2020 at 08:26:30PM +0200, Imre Deak wrote:
> > > > From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > >
> > > > Gen12 display can decompress surfaces compressed by render engine with
> > > > Clear Color, add a new modifier as the driver needs to know the surface
> > > > was compressed by render engine.
> > > >
> > > > V2: Description changes as suggested by Rafael.
> > > > V3: Mention the Clear Color size of 64 bits in the comments(DK)
> > > > v4: Fix trailing whitespaces
> > > > v5: Explain Clear Color in the documentation.
> > > > v6: Documentation Nitpicks(Nanley)
> > > >
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
> > > > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > Cc: Nanley Chery <nanley.g.chery@intel.com>
> > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  include/uapi/drm/drm_fourcc.h | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/include/uapi/drm/drm_fourcc.h
> b/include/uapi/drm/drm_fourcc.h
> > > > index ca48ed0e6bc1..0a1b2c4c4bee 100644
> > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > @@ -527,6 +527,25 @@ extern "C" {
> > > >   */
> > > >  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS
> fourcc_mod_code(INTEL, 7)
> > > >
> > > > +/*
> > > > + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
> > > > + * compression.
> > > > + *
> > > > + * The main surface is Y-tiled and is at plane index 0 whereas CCS is linear
> > > > + * and at index 1. The clear color is stored at index 2, and the pitch should
> > > > + * be ignored. The clear color structure is 256 bits. The first 128 bits
> > > > + * represents Raw Clear Color Red, Green, Blue and Alpha color each
> represented
> > > > + * by 32 bits. The raw clear color is consumed by the 3d engine and
> generates
> > > > + * the converted clear color of size 64 bits. The first 32 bits store the
> Lower
> > > > + * Converted Clear Color value and the next 32 bits store the Higher
> Converted
> > > > + * Clear Color value when applicable. The Converted Clear Color values
> are
> > > > + * consumed by the DE. The last 64 bits are used to store Color Discard
> Enable
> > > > + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache
> line
> > > > + * corresponds to an area of 4x1 tiles in the main surface. The main
> surface
> > > > + * pitch is required to be a multiple of 4 tile widths.
> > > > + */
> > > > +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC
> fourcc_mod_code(INTEL, 8)
> > > > +
> > > >  /*
> > > >   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
> > > >   *
> > > > --
> > > > 2.25.1
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
Imre Deak Dec. 1, 2020, 12:04 p.m. UTC | #9
Hi Nanley,

thanks for the review.

+Ville, Chris.

On Tue, Dec 01, 2020 at 02:18:26AM +0200, Chery, Nanley G wrote:
> Hi Imre,
>  
> I have a question and a couple comments:
> 
> Is the map of the clear color address creating a new synchronization
> point between the GPU and CPU? If so, I wonder how this will impact
> performance.

The kmap to read the clear value is not adding any sync overhead if
that's what you mean. But the clear value must be in place before we
read it out and that should be guaranteed by the flush we do anyway to wait
for the render result (even considering the explicit L3/RT flush, depth
stall the spec requires for fast clears).

However now that you mention: atm the kmap/readout happens after the
explicit but before the implicit fence-wait. I think it should happen
after the implicit fence-wait.

Ville, Chris, could you confirm the above and also that the above flush
is enough to ensure the CPU read is coherent?

> There was some talk of asynchronously updating the clear color
> register a while back. 

Couldn't find anything with a quick search, do you have a pointer? Just
before the flip we must wait for the render results anyway, as we do
now, so not sure how it could be optimized.

> We probably don't have to update the header, but we noticed in our
> testing that the clear color prefers an alignment greater than 64B.
> Unfortunately, I can't find any bspec note about this. As long as the
> buffer creators are aware though, I think we should be fine. I don't
> know if this is the best forum to bring it up, but I thought I'd
> share.

Yes, would be good to clarify this and get it also to the spec. Then the
driver should also check the alignment of the 3rd FB plane.

> Seems like the upper converted clear color is untested due to the lack
> of RGBX16 support. I suppose that if there are any issues there, they
> can be fixed later...

Yes, a 64bpp RC-CC subtest in IGT is missing, should be easy to add
that.

--Imre
Chery, Nanley G Dec. 11, 2020, 7:04 a.m. UTC | #10
> -----Original Message-----
> From: Imre Deak <imre.deak@intel.com>
> Sent: Tuesday, December 1, 2020 4:05 AM
> To: Chery, Nanley G <nanley.g.chery@intel.com>; Chris Wilson <chris@chris-
> wilson.co.uk>; Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>; intel-gfx@lists.freedesktop.org; Nikula,
> Jani <jani.nikula@intel.com>; Daniel Vetter <daniel.vetter@ffwll.ch>;
> Kondapally, Kalyan <kalyan.kondapally@intel.com>; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com>; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/framebuffer: Format modifier for
> Intel Gen 12 render compression with Clear Color
> 
> Hi Nanley,
> 
> thanks for the review.
> 
> +Ville, Chris.
> 
> On Tue, Dec 01, 2020 at 02:18:26AM +0200, Chery, Nanley G wrote:
> > Hi Imre,
> >
> > I have a question and a couple comments:
> >
> > Is the map of the clear color address creating a new synchronization
> > point between the GPU and CPU? If so, I wonder how this will impact
> > performance.
> 
> The kmap to read the clear value is not adding any sync overhead if
> that's what you mean. But the clear value must be in place before we
> read it out and that should be guaranteed by the flush we do anyway to wait
> for the render result (even considering the explicit L3/RT flush, depth
> stall the spec requires for fast clears).
> 
> However now that you mention: atm the kmap/readout happens after the
> explicit but before the implicit fence-wait. I think it should happen
> after the implicit fence-wait.
> 
> Ville, Chris, could you confirm the above and also that the above flush
> is enough to ensure the CPU read is coherent?
> 
> > There was some talk of asynchronously updating the clear color
> > register a while back.
> 
> Couldn't find anything with a quick search, do you have a pointer? Just
> before the flip we must wait for the render results anyway, as we do
> now, so not sure how it could be optimized.
> 
 
There were some offline discussions, so I don't have a reference unfortunately.
Though, given what you shared above it seems like it's actually not an issue.

> > We probably don't have to update the header, but we noticed in our
> > testing that the clear color prefers an alignment greater than 64B.
> > Unfortunately, I can't find any bspec note about this. As long as the
> > buffer creators are aware though, I think we should be fine. I don't
> > know if this is the best forum to bring it up, but I thought I'd
> > share.
> 
> Yes, would be good to clarify this and get it also to the spec. Then the
> driver should also check the alignment of the 3rd FB plane.
> 

I plan to run some more tests and file a bug in the spec.

I see that the IGT test only clears the fb once. Just to confirm, is the 
clear color offset read from on every frame? Userspace would like to be 
able to pass different clear colors for an fb. 

-Nanley

> > Seems like the upper converted clear color is untested due to the lack
> > of RGBX16 support. I suppose that if there are any issues there, they
> > can be fixed later...
> 
> Yes, a 64bpp RC-CC subtest in IGT is missing, should be easy to add
> that.
> 
> --Imre
Imre Deak Dec. 14, 2020, 4:22 p.m. UTC | #11
On Fri, Dec 11, 2020 at 09:04:02AM +0200, Chery, Nanley G wrote:
> [...]
> > > We probably don't have to update the header, but we noticed in our
> > > testing that the clear color prefers an alignment greater than 64B.
> > > Unfortunately, I can't find any bspec note about this. As long as the
> > > buffer creators are aware though, I think we should be fine. I don't
> > > know if this is the best forum to bring it up, but I thought I'd
> > > share.
> > 
> > Yes, would be good to clarify this and get it also to the spec. Then the
> > driver should also check the alignment of the 3rd FB plane.
> 
> I plan to run some more tests and file a bug in the spec.

Ok, thanks. Note that this patch has a problem with synchornization and
based on Chris' response I'm planning to update it once I figured out
the proper way to map the CC plane. Until that you could still use it on
TGL if you wait for the RT result explicitly after the fast clear and
before flipping to it (that's what the IGT test does atm).

> I see that the IGT test only clears the fb once. Just to confirm, is the 
> clear color offset read from on every frame? Userspace would like to be 
> able to pass different clear colors for an fb.

Yes, every time you do a flip the kernel will re-read the CC value and
program it to the display.

--Imre
diff mbox series

Patch

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index ca48ed0e6bc1..0a1b2c4c4bee 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -527,6 +527,25 @@  extern "C" {
  */
 #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
 
+/*
+ * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
+ * compression.
+ *
+ * The main surface is Y-tiled and is at plane index 0 whereas CCS is linear
+ * and at index 1. The clear color is stored at index 2, and the pitch should
+ * be ignored. The clear color structure is 256 bits. The first 128 bits
+ * represents Raw Clear Color Red, Green, Blue and Alpha color each represented
+ * by 32 bits. The raw clear color is consumed by the 3d engine and generates
+ * the converted clear color of size 64 bits. The first 32 bits store the Lower
+ * Converted Clear Color value and the next 32 bits store the Higher Converted
+ * Clear Color value when applicable. The Converted Clear Color values are
+ * consumed by the DE. The last 64 bits are used to store Color Discard Enable
+ * and Depth Clear Value Valid which are ignored by the DE. A CCS cache line
+ * corresponds to an area of 4x1 tiles in the main surface. The main surface
+ * pitch is required to be a multiple of 4 tile widths.
+ */
+#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
+
 /*
  * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
  *