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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 02/26/2015 04:44 PM, Daniel Vetter wrote:
> On Wed, Feb 25, 2015 at 04:47:24PM +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.
>>
>> v2: Validate passed in fb modifiers to reject garbage. (Daniel Vetter)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> (v1)
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1d50934..3232ddd 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12783,6 +12783,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>   	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>
>>   	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
>> +		/* Passed in modifier sanity checking. */
>> +		switch (mode_cmd->modifier[0]) {
>> +		case DRM_FORMAT_MOD_NONE:
>> +		case I915_FORMAT_MOD_X_TILED:
>> +		case I915_FORMAT_MOD_Y_TILED:
>> +		case I915_FORMAT_MOD_Yf_TILED:
>> +			break;
>> +		default:
>> +			DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
>> +				  mode_cmd->modifier[0]);
>> +			return -EINVAL;
>> +		}
>> +
>>   		/* Enforce that fb modifier and tiling mode match, but only for
>>   		 * X-tiled. This is needed for FBC. */
>>   		if (!!(obj->tiling_mode == I915_TILING_X) !=
>> @@ -12790,6 +12803,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;
>> @@ -12799,11 +12820,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;
>> -	}
>
> My idea was actually to put the switch here (reduces one indent level) and
> put the per-gen stuff into the switch statement too. I.e.
>
>      	/* Passed in modifier sanity checking. */
>      	switch (mode_cmd->modifier[0]) {
>      	case I915_FORMAT_MOD_Y_TILED:
>      	case I915_FORMAT_MOD_Yf_TILED:
> 		if (INTEL_INFO(dev)->gen < 9) {
> 			DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> 				  mode_cmd->modifier[0]);
> 			return -EINVAL;
>
> 		}
>      	case DRM_FORMAT_MOD_NONE:
>      	case I915_FORMAT_MOD_X_TILED:
>      		break;
>      	default:
>      		DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
>      			  mode_cmd->modifier[0]);
>      		return -EINVAL;
>      	}
>
> That gives us a natural place for extensions later on if we need to add
> more special cases.

Yes I agree this patch being only on v2 was way disproportionate 
compared to some others from this series. ;)

Regards,

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