[Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Tvrtko Ursulin]

Hi,

On 04/02/2015 05:54 AM, Jindal, Sonika wrote:
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index f0bbc22..86ee0f0 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view
>>> *view, struct drm_framebuffer *fb,
>>>           return -EINVAL;
>>>       }
>>>
>>> +    switch (fb->pixel_format) {
>>> +    case DRM_FORMAT_XRGB8888:
>>> +    case DRM_FORMAT_ARGB8888:
>>> +    case DRM_FORMAT_XBGR8888:
>>> +    case DRM_FORMAT_ABGR8888:
>>> +    case DRM_FORMAT_XRGB2101010:
>>> +    case DRM_FORMAT_ARGB2101010:
>>> +    case DRM_FORMAT_XBGR2101010:
>>> +    case DRM_FORMAT_ABGR2101010:
>>> +    case DRM_FORMAT_YUYV:
>>> +    case DRM_FORMAT_YVYU:
>>> +    case DRM_FORMAT_UYVY:
>>> +    case DRM_FORMAT_VYUY:
>>> +    case DRM_FORMAT_NV12:
>>> +        break;
>>> +
>>> +    default:
>>> +        DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270
>>> rotation!\n",
>>> +              fb->pixel_format);
>>> +        return -EINVAL;
>>> +    }
>>
>> Shouldn't we be matching against the list of formats the plane supports
>> (which may vary by platform, or by specific plane) rather than this
>> generic list?  We already specified what formats the plane can handle at
>> plane init time, so it seems like what you'd really want is just a call
>> to
>>
>>      drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format)
>>
>> then follow that up with explicit checks to exclude any formats that we
>> can handle in 0/180, but not in 90/270.
>>
> I am not sure how it will help. drm_plane_check_pixel_format should be
> used to check the pixel format of the fb which we should be doing in
> some -check functions (I don't think we do that right now?) against what
> is supported by the plane.
> But to check for the formats which are allowed for 90/270, we would need
> this kind of explicit check.
>
>> I'd also move this check to intel_plane_atomic_check(), since the
>> 'check' code path is where I'd usually go looking for these types of
>> checks; the function you've got it in at the moment gets called from the
>> 'prepare' step which works as well, but seems a bit less obvious.
>>
> Yes, I agree, but this is on top of Tvrtko's patch for secondary buffer
> mapping where based upon tiling and pixel format we are allowing the
> rotated gtt.
>
> Tvrtko,
> Can these be moved to the intel_plane_atomic_check()

Good point, I think it can and should. I suppose it was just an 
oversight during endless rebasing, that I put the Y tiling check in 
there. So you can move that part as well while you are doing it.

Also highlights the fact we have no negative testing in kms_rotation_crc 
for this. I mean, trying to rotate by 90/270 linear or X tiled, or wrong 
pixel format.

Regards,

Tvrtko