diff mbox

[7/7] drm/i915/skl: Allow Y (and Yf) frame buffer creation

Message ID 1424706961-27519-8-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Feb. 23, 2015, 3:56 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

By this patch all underlying bits have been implemented and this
patch actually enables the feature.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Lespiau, Damien Feb. 24, 2015, 7:30 p.m. UTC | #1 
On Mon, Feb 23, 2015 at 03:56:01PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> By this patch all underlying bits have been implemented and this
> patch actually enables the feature.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---

Ville noticed that it didn't seem we were properly returning -EINVAL
when userspace sets reserved bits in the modifiers field. Otherwise,
what's here seems to progress in the right direction:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Daniel Vetter Feb. 24, 2015, 9:46 p.m. UTC | #2 
On Mon, Feb 23, 2015 at 03:56:01PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> By this patch all underlying bits have been implemented and this
> patch actually enables the feature.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 74d4923..f100086 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12781,6 +12781,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
>  			return -EINVAL;
>  		}
> +
> +		if (INTEL_INFO(dev)->gen < 9 &&
> +		    (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> +		     mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> +			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> +				  mode_cmd->modifier[0]);
> +			return -EINVAL;
> +		}
>  	} else {
>  		if (obj->tiling_mode == I915_TILING_X)
>  			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> @@ -12790,11 +12798,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		}
>  	}
>  
> -	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
> -		DRM_DEBUG("hardware does not support tiling Y\n");
> -		return -EINVAL;
> -	}

Imo the clearer code would be to add a 
	
	switch (mode_cmd->modifier[0]) {

	}

here and then shovel the platform checks into the supgroups. At least
that's how we tend to roll these since it reduces the risks that a case is
forgotten when the enumeration is extended. That would also have caught
that we don't reject random garbage in the default: case.
-Daniel

> -
>  	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
>  						     mode_cmd->pixel_format);
>  	if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> -- 
> 2.3.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Shuang He Feb. 25, 2015, 6:08 a.m. UTC | #3 
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5814
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  281/281              281/281
ILK                                  305/305              305/305
SNB                 -1              326/326              325/326
IVB                                  380/380              380/380
BYT                                  294/294              294/294
HSW                 -1              421/421              420/421
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt_kms_plane_plane-position-hole-pipe-B-plane-1      DMESG_WARN(12)PASS(3)      DMESG_WARN(1)PASS(1)
 HSW  igt_gem_storedw_loop_vebox      DMESG_WARN(2)PASS(1)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(6)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
Tvrtko Ursulin Feb. 25, 2015, 10:51 a.m. UTC | #4 
On 02/24/2015 09:46 PM, Daniel Vetter wrote:
> On Mon, Feb 23, 2015 at 03:56:01PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> By this patch all underlying bits have been implemented and this
>> patch actually enables the feature.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 74d4923..f100086 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12781,6 +12781,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>   			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
>>   			return -EINVAL;
>>   		}
>> +
>> +		if (INTEL_INFO(dev)->gen < 9 &&
>> +		    (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>> +		     mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
>> +			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
>> +				  mode_cmd->modifier[0]);
>> +			return -EINVAL;
>> +		}
>>   	} else {
>>   		if (obj->tiling_mode == I915_TILING_X)
>>   			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
>> @@ -12790,11 +12798,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>   		}
>>   	}
>>
>> -	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
>> -		DRM_DEBUG("hardware does not support tiling Y\n");
>> -		return -EINVAL;
>> -	}
>
> Imo the clearer code would be to add a
> 	
> 	switch (mode_cmd->modifier[0]) {
>
> 	}
>
> here and then shovel the platform checks into the supgroups. At least
> that's how we tend to roll these since it reduces the risks that a case is
> forgotten when the enumeration is extended. That would also have caught
> that we don't reject random garbage in the default: case.

Yes that's true - I'll see what I can do.

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 74d4923..f100086 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12781,6 +12781,14 @@  static int intel_framebuffer_init(struct drm_device *dev,
 			DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
 			return -EINVAL;
 		}
+
+		if (INTEL_INFO(dev)->gen < 9 &&
+		    (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
+		     mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
+			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
+				  mode_cmd->modifier[0]);
+			return -EINVAL;
+		}
 	} else {
 		if (obj->tiling_mode == I915_TILING_X)
 			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
@@ -12790,11 +12798,6 @@  static int intel_framebuffer_init(struct drm_device *dev,
 		}
 	}
 
-	if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
-		DRM_DEBUG("hardware does not support tiling Y\n");
-		return -EINVAL;
-	}
-
 	stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
 						     mode_cmd->pixel_format);
 	if (mode_cmd->pitches[0] & (stride_alignment - 1)) {