[PATCH 1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[PATCH 1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[Paulo Zanoni]

Function intel_framebuffer_init() checks for the possibilities during
framebuffer creation (addfb ioctl time). It is missing the fact that
the indexed format is not supported with Yf tiling.

It is worth noticing that skl_plane_format_mod_supported() correctly
handles for the C8/Yf combination, but this function runs during
modeset time, so we only reject the combination later.

Ville recently proposed a new IGT test that only uses addfb to assert
supported formats, so that IGT was failing. Add the check so we get
green squares right from the start after Ville merges his test.

Also drive-by fix the missing /* fall through */ in the chunk we
modified by just turning it into a "break;" since IMHO breaks are
easier to read than fall-throughs.

BSpec: 18565
Testcase: igt/kms_addfb_basic/expected-formats (not merged yet)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index eb25037d7b38..fdff1779f778 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 			goto err;
 		}
 		/* fall through */
-	case I915_FORMAT_MOD_Y_TILED:
 	case I915_FORMAT_MOD_Yf_TILED:
+		if (mode_cmd->pixel_format == DRM_FORMAT_C8) {
+			DRM_DEBUG_KMS("Indexed format does not support Yf tiling\n");
+			goto err;
+		}
+		/* fall through */
+	case I915_FORMAT_MOD_Y_TILED:
 		if (INTEL_GEN(dev_priv) < 9) {
 			DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n",
 				      mode_cmd->modifier[0]);
 			goto err;
 		}
+		break;
 	case DRM_FORMAT_MOD_LINEAR:
 	case I915_FORMAT_MOD_X_TILED:
 		break;
-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: make the primary plane func structs const

[Paulo Zanoni]

Because we can, the places where we use them already expect const
structs.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fdff1779f778..a21ec8ae46f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13467,7 +13467,7 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
 		format == DRM_FORMAT_ARGB8888;
 }
 
-static struct drm_plane_funcs skl_plane_funcs = {
+static const struct drm_plane_funcs skl_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
 	.destroy = intel_plane_destroy,
@@ -13478,7 +13478,7 @@ static struct drm_plane_funcs skl_plane_funcs = {
 	.format_mod_supported = skl_plane_format_mod_supported,
 };
 
-static struct drm_plane_funcs i965_plane_funcs = {
+static const struct drm_plane_funcs i965_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
 	.destroy = intel_plane_destroy,
@@ -13489,7 +13489,7 @@ static struct drm_plane_funcs i965_plane_funcs = {
 	.format_mod_supported = i965_plane_format_mod_supported,
 };
 
-static struct drm_plane_funcs i8xx_plane_funcs = {
+static const struct drm_plane_funcs i8xx_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
 	.destroy = intel_plane_destroy,
-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: remove a copy of skl_plane_format_mod_supported()

[Paulo Zanoni]

A little git-blame'ing suggests that both functions were added by
commit 714244e280de ("drm/i915: Add format modifiers for Intel"), as
skl_mod_supported() on intel_display.c and
skl_plane_format_mod_supported() on intel_sprite.c. At that time they
were different, but right now they are exactly the same, name and tabs
included. Remove one of the copies and make the other non-static.

Given how our hardware has evolved it is unlikely that future
platforms will require a different format_mod_supported() function for
the primary plane vs the sprite plane, and we can always bring back
the additional copy if we need.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 50 ------------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_sprite.c  |  4 +--
 3 files changed, 4 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a21ec8ae46f6..b9a0183dc580 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13410,56 +13410,6 @@ static bool i965_plane_format_mod_supported(struct drm_plane *_plane,
 	}
 }
 
-static bool skl_plane_format_mod_supported(struct drm_plane *_plane,
-					   u32 format, u64 modifier)
-{
-	struct intel_plane *plane = to_intel_plane(_plane);
-
-	switch (modifier) {
-	case DRM_FORMAT_MOD_LINEAR:
-	case I915_FORMAT_MOD_X_TILED:
-	case I915_FORMAT_MOD_Y_TILED:
-	case I915_FORMAT_MOD_Yf_TILED:
-		break;
-	case I915_FORMAT_MOD_Y_TILED_CCS:
-	case I915_FORMAT_MOD_Yf_TILED_CCS:
-		if (!plane->has_ccs)
-			return false;
-		break;
-	default:
-		return false;
-	}
-
-	switch (format) {
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_XBGR8888:
-	case DRM_FORMAT_ARGB8888:
-	case DRM_FORMAT_ABGR8888:
-		if (is_ccs_modifier(modifier))
-			return true;
-		/* fall through */
-	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_XRGB2101010:
-	case DRM_FORMAT_XBGR2101010:
-	case DRM_FORMAT_YUYV:
-	case DRM_FORMAT_YVYU:
-	case DRM_FORMAT_UYVY:
-	case DRM_FORMAT_VYUY:
-	case DRM_FORMAT_NV12:
-		if (modifier == I915_FORMAT_MOD_Yf_TILED)
-			return true;
-		/* fall through */
-	case DRM_FORMAT_C8:
-		if (modifier == DRM_FORMAT_MOD_LINEAR ||
-		    modifier == I915_FORMAT_MOD_X_TILED ||
-		    modifier == I915_FORMAT_MOD_Y_TILED)
-			return true;
-		/* fall through */
-	default:
-		return false;
-	}
-}
-
 static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
 					      u32 format, u64 modifier)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bf1c38728a59..313337f03da8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2140,6 +2140,8 @@ unsigned int skl_plane_max_stride(struct intel_plane *plane,
 				  unsigned int rotation);
 int skl_plane_check(struct intel_crtc_state *crtc_state,
 		    struct intel_plane_state *plane_state);
+bool skl_plane_format_mod_supported(struct drm_plane *_plane,
+				    u32 format, u64 modifier);
 int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state);
 int chv_plane_check_rotation(const struct intel_plane_state *plane_state);
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8821e59b70ea..db297d257e1e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1609,8 +1609,8 @@ static bool vlv_sprite_format_mod_supported(struct drm_plane *_plane,
 	}
 }
 
-static bool skl_plane_format_mod_supported(struct drm_plane *_plane,
-					   u32 format, u64 modifier)
+bool skl_plane_format_mod_supported(struct drm_plane *_plane,
+				    u32 format, u64 modifier)
 {
 	struct intel_plane *plane = to_intel_plane(_plane);
 
-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling
URL   : https://patchwork.freedesktop.org/series/50115/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4869 -> Patchwork_10268 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50115/revisions/1/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-bdw-samus:       PASS -> INCOMPLETE (fdo#107773)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7560u:       INCOMPLETE (fdo#108044) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-WARN (fdo#102614) -> PASS

    igt@pm_rpm@module-reload:
      fi-skl-caroline:    INCOMPLETE (fdo#107807) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#108044 https://bugs.freedesktop.org/show_bug.cgi?id=108044


== Participating hosts (46 -> 42) ==

  Additional (1): fi-skl-guc 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4869 -> Patchwork_10268

  CI_DRM_4869: 9a74a6db272a007c3db063ae3375fbee60a7bd53 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10268: 7ea4513bb3c512c1045ed3e253166d213f2172db @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7ea4513bb3c5 drm/i915: remove a copy of skl_plane_format_mod_supported()
03ad98ef111e drm/i915: make the primary plane func structs const
f1df62edc741 drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10268/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling
URL   : https://patchwork.freedesktop.org/series/50115/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4869_full -> Patchwork_10268_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10268_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10268_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS
      shard-snb:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
      shard-snb:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_setmode@basic:
      shard-snb:          NOTRUN -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@gem_exec_schedule@smoketest-render:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
      shard-apl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +12

    
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4869 -> Patchwork_10268

  CI_DRM_4869: 9a74a6db272a007c3db063ae3375fbee60a7bd53 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10268: 7ea4513bb3c512c1045ed3e253166d213f2172db @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10268/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: remove a copy of skl_plane_format_mod_supported()

[dhinakaran.pandiyan]

On Mon, 2018-09-24 at 17:19 -0700, Paulo Zanoni wrote:
> A little git-blame'ing suggests that both functions were added by
> commit 714244e280de ("drm/i915: Add format modifiers for Intel"), as
> skl_mod_supported() on intel_display.c and
> skl_plane_format_mod_supported() on intel_sprite.c. At that time they
> were different, but right now they are exactly the same, name and
> tabs
> included. Remove one of the copies and make the other non-static.
> 
> Given how our hardware has evolved it is unlikely that future
> platforms will require a different format_mod_supported() function
> for
> the primary plane vs the sprite plane, and we can always bring
> back
> the additional copy if we need.

Quick review since I knew the function had a duplicate.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Also noticed that the modifier arrays are exact copies but the pixel
format arrays look different. 


> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 50 --------------------------
> ----------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_sprite.c  |  4 +--
>  3 files changed, 4 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a21ec8ae46f6..b9a0183dc580 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13410,56 +13410,6 @@ static bool
> i965_plane_format_mod_supported(struct drm_plane *_plane,
>  	}
>  }
>  
> -static bool skl_plane_format_mod_supported(struct drm_plane *_plane,
> -					   u32 format, u64 modifier)
> -{
> -	struct intel_plane *plane = to_intel_plane(_plane);
> -
> -	switch (modifier) {
> -	case DRM_FORMAT_MOD_LINEAR:
> -	case I915_FORMAT_MOD_X_TILED:
> -	case I915_FORMAT_MOD_Y_TILED:
> -	case I915_FORMAT_MOD_Yf_TILED:
> -		break;
> -	case I915_FORMAT_MOD_Y_TILED_CCS:
> -	case I915_FORMAT_MOD_Yf_TILED_CCS:
> -		if (!plane->has_ccs)
> -			return false;
> -		break;
> -	default:
> -		return false;
> -	}
> -
> -	switch (format) {
> -	case DRM_FORMAT_XRGB8888:
> -	case DRM_FORMAT_XBGR8888:
> -	case DRM_FORMAT_ARGB8888:
> -	case DRM_FORMAT_ABGR8888:
> -		if (is_ccs_modifier(modifier))
> -			return true;
> -		/* fall through */
> -	case DRM_FORMAT_RGB565:
> -	case DRM_FORMAT_XRGB2101010:
> -	case DRM_FORMAT_XBGR2101010:
> -	case DRM_FORMAT_YUYV:
> -	case DRM_FORMAT_YVYU:
> -	case DRM_FORMAT_UYVY:
> -	case DRM_FORMAT_VYUY:
> -	case DRM_FORMAT_NV12:
> -		if (modifier == I915_FORMAT_MOD_Yf_TILED)
> -			return true;
> -		/* fall through */
> -	case DRM_FORMAT_C8:
> -		if (modifier == DRM_FORMAT_MOD_LINEAR ||
> -		    modifier == I915_FORMAT_MOD_X_TILED ||
> -		    modifier == I915_FORMAT_MOD_Y_TILED)
> -			return true;
> -		/* fall through */
> -	default:
> -		return false;
> -	}
> -}
> -
>  static bool intel_cursor_format_mod_supported(struct drm_plane
> *_plane,
>  					      u32 format, u64
> modifier)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index bf1c38728a59..313337f03da8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2140,6 +2140,8 @@ unsigned int skl_plane_max_stride(struct
> intel_plane *plane,
>  				  unsigned int rotation);
>  int skl_plane_check(struct intel_crtc_state *crtc_state,
>  		    struct intel_plane_state *plane_state);
> +bool skl_plane_format_mod_supported(struct drm_plane *_plane,
> +				    u32 format, u64 modifier);
>  int intel_plane_check_src_coordinates(struct intel_plane_state
> *plane_state);
>  int chv_plane_check_rotation(const struct intel_plane_state
> *plane_state);
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 8821e59b70ea..db297d257e1e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1609,8 +1609,8 @@ static bool
> vlv_sprite_format_mod_supported(struct drm_plane *_plane,
>  	}
>  }
>  
> -static bool skl_plane_format_mod_supported(struct drm_plane *_plane,
> -					   u32 format, u64 modifier)
> +bool skl_plane_format_mod_supported(struct drm_plane *_plane,
> +				    u32 format, u64 modifier)
>  {
>  	struct intel_plane *plane = to_intel_plane(_plane);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[Ville Syrjälä]

On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote:
> Function intel_framebuffer_init() checks for the possibilities during
> framebuffer creation (addfb ioctl time). It is missing the fact that
> the indexed format is not supported with Yf tiling.
> 
> It is worth noticing that skl_plane_format_mod_supported() correctly
> handles for the C8/Yf combination, but this function runs during
> modeset time, so we only reject the combination later.
> 
> Ville recently proposed a new IGT test that only uses addfb to assert
> supported formats, so that IGT was failing. Add the check so we get
> green squares right from the start after Ville merges his test.

I have two of three (possibly) nicer ways to solve this:
https://patchwork.freedesktop.org/series/39700/
https://patchwork.freedesktop.org/series/39383/
https://patchwork.freedesktop.org/series/39813/

Would be nice if someone could figure out a solution (one of those or
perhaps some other solution I didn't think of) that enough people are
willing to accept.

> 
> Also drive-by fix the missing /* fall through */ in the chunk we
> modified by just turning it into a "break;" since IMHO breaks are
> easier to read than fall-throughs.
> 
> BSpec: 18565
> Testcase: igt/kms_addfb_basic/expected-formats (not merged yet)
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index eb25037d7b38..fdff1779f778 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  			goto err;
>  		}
>  		/* fall through */
> -	case I915_FORMAT_MOD_Y_TILED:
>  	case I915_FORMAT_MOD_Yf_TILED:
> +		if (mode_cmd->pixel_format == DRM_FORMAT_C8) {
> +			DRM_DEBUG_KMS("Indexed format does not support Yf tiling\n");
> +			goto err;
> +		}
> +		/* fall through */
> +	case I915_FORMAT_MOD_Y_TILED:
>  		if (INTEL_GEN(dev_priv) < 9) {
>  			DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n",
>  				      mode_cmd->modifier[0]);
>  			goto err;
>  		}
> +		break;
>  	case DRM_FORMAT_MOD_LINEAR:
>  	case I915_FORMAT_MOD_X_TILED:
>  		break;
> -- 
> 2.14.4

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: make the primary plane func structs const

[Ville Syrjälä]

On Mon, Sep 24, 2018 at 05:19:12PM -0700, Paulo Zanoni wrote:
> Because we can, the places where we use them already expect const
> structs.

https://patchwork.freedesktop.org/series/44104/ already has the rest of
the series covered. It didn't get pushed due to lack of drm-misc
backmerge. I suppose that's no longer an issue, but now the series
most likely needs a rebase.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fdff1779f778..a21ec8ae46f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13467,7 +13467,7 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
>  		format == DRM_FORMAT_ARGB8888;
>  }
>  
> -static struct drm_plane_funcs skl_plane_funcs = {
> +static const struct drm_plane_funcs skl_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
>  	.destroy = intel_plane_destroy,
> @@ -13478,7 +13478,7 @@ static struct drm_plane_funcs skl_plane_funcs = {
>  	.format_mod_supported = skl_plane_format_mod_supported,
>  };
>  
> -static struct drm_plane_funcs i965_plane_funcs = {
> +static const struct drm_plane_funcs i965_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
>  	.destroy = intel_plane_destroy,
> @@ -13489,7 +13489,7 @@ static struct drm_plane_funcs i965_plane_funcs = {
>  	.format_mod_supported = i965_plane_format_mod_supported,
>  };
>  
> -static struct drm_plane_funcs i8xx_plane_funcs = {
> +static const struct drm_plane_funcs i8xx_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
>  	.destroy = intel_plane_destroy,
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[Paulo Zanoni]

Em Ter, 2018-09-25 às 15:02 +0300, Ville Syrjälä escreveu:
> On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote:
> > Function intel_framebuffer_init() checks for the possibilities
> > during
> > framebuffer creation (addfb ioctl time). It is missing the fact
> > that
> > the indexed format is not supported with Yf tiling.
> > 
> > It is worth noticing that skl_plane_format_mod_supported()
> > correctly
> > handles for the C8/Yf combination, but this function runs during
> > modeset time, so we only reject the combination later.
> > 
> > Ville recently proposed a new IGT test that only uses addfb to
> > assert
> > supported formats, so that IGT was failing. Add the check so we get
> > green squares right from the start after Ville merges his test.
> 
> I have two of three (possibly) nicer ways to solve this:
> https://patchwork.freedesktop.org/series/39700/

I thought about implementing this one when looking at the code. I agree
the duplicated checks are horrible. I thought maybe this model wouldn't
be acceptable due to the inefficiency of always looping over everything
vs the current linear solution.

I see no review comments on this series besides the vc4 patch. Did you
get anything that's not appearing on patchwork?

> https://patchwork.freedesktop.org/series/39383/

You have blocked your own patch with your own review here.

> https://patchwork.freedesktop.org/series/39813/

Looks like there's some potential controversy to be untangled here if
we wish to follow this route.

> solution 4

I guess it would be to simply not have the checks at all. But this
would be an interface change instead of just refactoring code
duplication.


> 
> Would be nice if someone could figure out a solution (one of those or
> perhaps some other solution I didn't think of) that enough people are
> willing to accept.

I could see solution 1 moving forward more easily, and even could
volunteer myself to review a rebased version.

In the meantime, we could actually review/commit this immediate fix and
have a correct-but-not-yet-reworked codebase instead of waiting for a
patch that has been abandoned since March. I don't think one series
should block the other.


> 
> > 
> > Also drive-by fix the missing /* fall through */ in the chunk we
> > modified by just turning it into a "break;" since IMHO breaks are
> > easier to read than fall-throughs.
> > 
> > BSpec: 18565
> > Testcase: igt/kms_addfb_basic/expected-formats (not merged yet)
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index eb25037d7b38..fdff1779f778 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> >  			goto err;
> >  		}
> >  		/* fall through */
> > -	case I915_FORMAT_MOD_Y_TILED:
> >  	case I915_FORMAT_MOD_Yf_TILED:
> > +		if (mode_cmd->pixel_format == DRM_FORMAT_C8) {
> > +			DRM_DEBUG_KMS("Indexed format does not
> > support Yf tiling\n");
> > +			goto err;
> > +		}
> > +		/* fall through */
> > +	case I915_FORMAT_MOD_Y_TILED:
> >  		if (INTEL_GEN(dev_priv) < 9) {
> >  			DRM_DEBUG_KMS("Unsupported tiling
> > 0x%llx!\n",
> >  				      mode_cmd->modifier[0]);
> >  			goto err;
> >  		}
> > +		break;
> >  	case DRM_FORMAT_MOD_LINEAR:
> >  	case I915_FORMAT_MOD_X_TILED:
> >  		break;
> > -- 
> > 2.14.4
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: make the primary plane func structs const

[Paulo Zanoni]

Em Ter, 2018-09-25 às 15:05 +0300, Ville Syrjälä escreveu:
> On Mon, Sep 24, 2018 at 05:19:12PM -0700, Paulo Zanoni wrote:
> > Because we can, the places where we use them already expect const
> > structs.
> 
> https://patchwork.freedesktop.org/series/44104/ already has the rest
> of
> the series covered. It didn't get pushed due to lack of drm-misc
> backmerge. I suppose that's no longer an issue, but now the series
> most likely needs a rebase.

Please try to move forward with what's already reviewed so we can
prevent a third person from reimplementing these improvements :).


> 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index fdff1779f778..a21ec8ae46f6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13467,7 +13467,7 @@ static bool
> > intel_cursor_format_mod_supported(struct drm_plane *_plane,
> >  		format == DRM_FORMAT_ARGB8888;
> >  }
> >  
> > -static struct drm_plane_funcs skl_plane_funcs = {
> > +static const struct drm_plane_funcs skl_plane_funcs = {
> >  	.update_plane = drm_atomic_helper_update_plane,
> >  	.disable_plane = drm_atomic_helper_disable_plane,
> >  	.destroy = intel_plane_destroy,
> > @@ -13478,7 +13478,7 @@ static struct drm_plane_funcs
> > skl_plane_funcs = {
> >  	.format_mod_supported = skl_plane_format_mod_supported,
> >  };
> >  
> > -static struct drm_plane_funcs i965_plane_funcs = {
> > +static const struct drm_plane_funcs i965_plane_funcs = {
> >  	.update_plane = drm_atomic_helper_update_plane,
> >  	.disable_plane = drm_atomic_helper_disable_plane,
> >  	.destroy = intel_plane_destroy,
> > @@ -13489,7 +13489,7 @@ static struct drm_plane_funcs
> > i965_plane_funcs = {
> >  	.format_mod_supported = i965_plane_format_mod_supported,
> >  };
> >  
> > -static struct drm_plane_funcs i8xx_plane_funcs = {
> > +static const struct drm_plane_funcs i8xx_plane_funcs = {
> >  	.update_plane = drm_atomic_helper_update_plane,
> >  	.disable_plane = drm_atomic_helper_disable_plane,
> >  	.destroy = intel_plane_destroy,
> > -- 
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[Ville Syrjälä]

On Tue, Sep 25, 2018 at 03:02:21PM -0700, Paulo Zanoni wrote:
> Em Ter, 2018-09-25 às 15:02 +0300, Ville Syrjälä escreveu:
> > On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote:
> > > Function intel_framebuffer_init() checks for the possibilities
> > > during
> > > framebuffer creation (addfb ioctl time). It is missing the fact
> > > that
> > > the indexed format is not supported with Yf tiling.
> > > 
> > > It is worth noticing that skl_plane_format_mod_supported()
> > > correctly
> > > handles for the C8/Yf combination, but this function runs during
> > > modeset time, so we only reject the combination later.
> > > 
> > > Ville recently proposed a new IGT test that only uses addfb to
> > > assert
> > > supported formats, so that IGT was failing. Add the check so we get
> > > green squares right from the start after Ville merges his test.
> > 
> > I have two of three (possibly) nicer ways to solve this:
> > https://patchwork.freedesktop.org/series/39700/
> 
> I thought about implementing this one when looking at the code. I agree
> the duplicated checks are horrible. I thought maybe this model wouldn't
> be acceptable due to the inefficiency of always looping over everything
> vs the current linear solution.
> 
> I see no review comments on this series besides the vc4 patch. Did you
> get anything that's not appearing on patchwork?
> 
> > https://patchwork.freedesktop.org/series/39383/
> 
> You have blocked your own patch with your own review here.
> 
> > https://patchwork.freedesktop.org/series/39813/
> 
> Looks like there's some potential controversy to be untangled here if
> we wish to follow this route.
> 
> > solution 4
> 
> I guess it would be to simply not have the checks at all. But this
> would be an interface change instead of just refactoring code
> duplication.
> 
> 
> > 
> > Would be nice if someone could figure out a solution (one of those or
> > perhaps some other solution I didn't think of) that enough people are
> > willing to accept.
> 
> I could see solution 1 moving forward more easily, and even could
> volunteer myself to review a rebased version.
> 
> In the meantime, we could actually review/commit this immediate fix and
> have a correct-but-not-yet-reworked codebase instead of waiting for a
> patch that has been abandoned since March. I don't think one series
> should block the other.

Sure. Was just trying to trick someone into finishing what I started ;)

Patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> 
> > 
> > > 
> > > Also drive-by fix the missing /* fall through */ in the chunk we
> > > modified by just turning it into a "break;" since IMHO breaks are
> > > easier to read than fall-throughs.
> > > 
> > > BSpec: 18565
> > > Testcase: igt/kms_addfb_basic/expected-formats (not merged yet)
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index eb25037d7b38..fdff1779f778 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct
> > > intel_framebuffer *intel_fb,
> > >  			goto err;
> > >  		}
> > >  		/* fall through */
> > > -	case I915_FORMAT_MOD_Y_TILED:
> > >  	case I915_FORMAT_MOD_Yf_TILED:
> > > +		if (mode_cmd->pixel_format == DRM_FORMAT_C8) {
> > > +			DRM_DEBUG_KMS("Indexed format does not
> > > support Yf tiling\n");
> > > +			goto err;
> > > +		}
> > > +		/* fall through */
> > > +	case I915_FORMAT_MOD_Y_TILED:
> > >  		if (INTEL_GEN(dev_priv) < 9) {
> > >  			DRM_DEBUG_KMS("Unsupported tiling
> > > 0x%llx!\n",
> > >  				      mode_cmd->modifier[0]);
> > >  			goto err;
> > >  		}
> > > +		break;
> > >  	case DRM_FORMAT_MOD_LINEAR:
> > >  	case I915_FORMAT_MOD_X_TILED:
> > >  		break;
> > > -- 
> > > 2.14.4
> > 
> > 

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling

[Paulo Zanoni]

Em Qui, 2018-09-27 às 17:16 +0300, Ville Syrjälä escreveu:
> On Tue, Sep 25, 2018 at 03:02:21PM -0700, Paulo Zanoni wrote:
> > Em Ter, 2018-09-25 às 15:02 +0300, Ville Syrjälä escreveu:
> > > On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote:
> > > > Function intel_framebuffer_init() checks for the possibilities
> > > > during
> > > > framebuffer creation (addfb ioctl time). It is missing the fact
> > > > that
> > > > the indexed format is not supported with Yf tiling.
> > > > 
> > > > It is worth noticing that skl_plane_format_mod_supported()
> > > > correctly
> > > > handles for the C8/Yf combination, but this function runs
> > > > during
> > > > modeset time, so we only reject the combination later.
> > > > 
> > > > Ville recently proposed a new IGT test that only uses addfb to
> > > > assert
> > > > supported formats, so that IGT was failing. Add the check so we
> > > > get
> > > > green squares right from the start after Ville merges his test.
> > > 
> > > I have two of three (possibly) nicer ways to solve this:
> > > https://patchwork.freedesktop.org/series/39700/
> > 
> > I thought about implementing this one when looking at the code. I
> > agree
> > the duplicated checks are horrible. I thought maybe this model
> > wouldn't
> > be acceptable due to the inefficiency of always looping over
> > everything
> > vs the current linear solution.
> > 
> > I see no review comments on this series besides the vc4 patch. Did
> > you
> > get anything that's not appearing on patchwork?
> > 
> > > https://patchwork.freedesktop.org/series/39383/
> > 
> > You have blocked your own patch with your own review here.
> > 
> > > https://patchwork.freedesktop.org/series/39813/
> > 
> > Looks like there's some potential controversy to be untangled here
> > if
> > we wish to follow this route.
> > 
> > > solution 4
> > 
> > I guess it would be to simply not have the checks at all. But this
> > would be an interface change instead of just refactoring code
> > duplication.
> > 
> > 
> > > 
> > > Would be nice if someone could figure out a solution (one of
> > > those or
> > > perhaps some other solution I didn't think of) that enough people
> > > are
> > > willing to accept.
> > 
> > I could see solution 1 moving forward more easily, and even could
> > volunteer myself to review a rebased version.
> > 
> > In the meantime, we could actually review/commit this immediate fix
> > and
> > have a correct-but-not-yet-reworked codebase instead of waiting for
> > a
> > patch that has been abandoned since March. I don't think one series
> > should block the other.
> 
> Sure. Was just trying to trick someone into finishing what I started
> ;)
> 
> Patch is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks a lot! Feel free to CC me on the series that kills the whole
thing, I agree it's a good thing to do.

> 
> > 
> > 
> > > 
> > > > 
> > > > Also drive-by fix the missing /* fall through */ in the chunk
> > > > we
> > > > modified by just turning it into a "break;" since IMHO breaks
> > > > are
> > > > easier to read than fall-throughs.
> > > > 
> > > > BSpec: 18565
> > > > Testcase: igt/kms_addfb_basic/expected-formats (not merged yet)
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index eb25037d7b38..fdff1779f778 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -14473,13 +14473,19 @@ static int
> > > > intel_framebuffer_init(struct
> > > > intel_framebuffer *intel_fb,
> > > >  			goto err;
> > > >  		}
> > > >  		/* fall through */
> > > > -	case I915_FORMAT_MOD_Y_TILED:
> > > >  	case I915_FORMAT_MOD_Yf_TILED:
> > > > +		if (mode_cmd->pixel_format == DRM_FORMAT_C8) {
> > > > +			DRM_DEBUG_KMS("Indexed format does not
> > > > support Yf tiling\n");
> > > > +			goto err;
> > > > +		}
> > > > +		/* fall through */
> > > > +	case I915_FORMAT_MOD_Y_TILED:
> > > >  		if (INTEL_GEN(dev_priv) < 9) {
> > > >  			DRM_DEBUG_KMS("Unsupported tiling
> > > > 0x%llx!\n",
> > > >  				      mode_cmd->modifier[0]);
> > > >  			goto err;
> > > >  		}
> > > > +		break;
> > > >  	case DRM_FORMAT_MOD_LINEAR:
> > > >  	case I915_FORMAT_MOD_X_TILED:
> > > >  		break;
> > > > -- 
> > > > 2.14.4
> > > 
> > > 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx