[Intel-gfx] [PATCH] drm/i915: Add plane .{min, max}_width() and .max_height() vfuncs
Aditya Swarup
aditya.swarup at intel.com
Fri Nov 13 16:05:49 UTC 2020
On 10/30/20 5:37 AM, Ville Syrjälä wrote:
> On Thu, Oct 22, 2020 at 05:17:00PM -0700, Aditya Swarup wrote:
>> On 10/16/20 4:40 PM, Aditya Swarup wrote:
>>> On 9/24/20 11:51 AM, Ville Syrjala wrote:
>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>
>>>> Reduce this maintenance nightmare a bit by converting the plane
>>>> min/max width/height stuff into vfuncs.
>>>>
>>>> Now, if I could just think of a nice way to also use this for
>>>> intel_mode_valid_max_plane_size()...
>>>>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> LGTM..
>>> Reviewed-by: Aditya Swarup <aditya.swarup at intel.com>
>> Hi Ville
>>
>> Are you going to push this patch to drm-tip or are you planning to rework this patch?
>> This patch simplifies the max/min plane width plane assignment and fixes the NV12 aux surface bug
>> and is good enough to push.
>
> Didn't you have a related fix you wanted to get in first?
My patch is still unreviewed - https://patchwork.freedesktop.org/patch/394819/ and your patch fixes that
issue as well in a cleaner way.
So, I don't think it is worth the effort to push my patch and then rebase your patch. This patch should
be sufficient.
Aditya Swarup
>
>>
>> Regards,
>> Aditya Swarup
>>>> ---
>>>> drivers/gpu/drm/i915/display/intel_display.c | 194 +++++-------------
>>>> .../drm/i915/display/intel_display_types.h | 9 +
>>>> drivers/gpu/drm/i915/display/intel_sprite.c | 140 +++++++++++++
>>>> 3 files changed, 196 insertions(+), 147 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>> index 5a9d933e425a..a823d406f0ee 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>> @@ -3696,127 +3696,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>>>> &to_intel_frontbuffer(fb)->bits);
>>>> }
>>>>
>>>> -static int skl_max_plane_width(const struct drm_framebuffer *fb,
>>>> - int color_plane,
>>>> - unsigned int rotation)
>>>> -{
>>>> - int cpp = fb->format->cpp[color_plane];
>>>> -
>>>> - switch (fb->modifier) {
>>>> - case DRM_FORMAT_MOD_LINEAR:
>>>> - case I915_FORMAT_MOD_X_TILED:
>>>> - /*
>>>> - * Validated limit is 4k, but has 5k should
>>>> - * work apart from the following features:
>>>> - * - Ytile (already limited to 4k)
>>>> - * - FP16 (already limited to 4k)
>>>> - * - render compression (already limited to 4k)
>>>> - * - KVMR sprite and cursor (don't care)
>>>> - * - horizontal panning (TODO verify this)
>>>> - * - pipe and plane scaling (TODO verify this)
>>>> - */
>>>> - if (cpp == 8)
>>>> - return 4096;
>>>> - else
>>>> - return 5120;
>>>> - case I915_FORMAT_MOD_Y_TILED_CCS:
>>>> - case I915_FORMAT_MOD_Yf_TILED_CCS:
>>>> - case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
>>>> - /* FIXME AUX plane? */
>>>> - case I915_FORMAT_MOD_Y_TILED:
>>>> - case I915_FORMAT_MOD_Yf_TILED:
>>>> - if (cpp == 8)
>>>> - return 2048;
>>>> - else
>>>> - return 4096;
>>>> - default:
>>>> - MISSING_CASE(fb->modifier);
>>>> - return 2048;
>>>> - }
>>>> -}
>>>> -
>>>> -static int glk_max_plane_width(const struct drm_framebuffer *fb,
>>>> - int color_plane,
>>>> - unsigned int rotation)
>>>> -{
>>>> - int cpp = fb->format->cpp[color_plane];
>>>> -
>>>> - switch (fb->modifier) {
>>>> - case DRM_FORMAT_MOD_LINEAR:
>>>> - case I915_FORMAT_MOD_X_TILED:
>>>> - if (cpp == 8)
>>>> - return 4096;
>>>> - else
>>>> - return 5120;
>>>> - case I915_FORMAT_MOD_Y_TILED_CCS:
>>>> - case I915_FORMAT_MOD_Yf_TILED_CCS:
>>>> - /* FIXME AUX plane? */
>>>> - case I915_FORMAT_MOD_Y_TILED:
>>>> - case I915_FORMAT_MOD_Yf_TILED:
>>>> - if (cpp == 8)
>>>> - return 2048;
>>>> - else
>>>> - return 5120;
>>>> - default:
>>>> - MISSING_CASE(fb->modifier);
>>>> - return 2048;
>>>> - }
>>>> -}
>>>> -
>>>> -static int icl_min_plane_width(const struct drm_framebuffer *fb)
>>>> -{
>>>> - /* Wa_14011264657, Wa_14011050563: gen11+ */
>>>> - switch (fb->format->format) {
>>>> - case DRM_FORMAT_C8:
>>>> - return 18;
>>>> - case DRM_FORMAT_RGB565:
>>>> - return 10;
>>>> - case DRM_FORMAT_XRGB8888:
>>>> - case DRM_FORMAT_XBGR8888:
>>>> - case DRM_FORMAT_ARGB8888:
>>>> - case DRM_FORMAT_ABGR8888:
>>>> - case DRM_FORMAT_XRGB2101010:
>>>> - case DRM_FORMAT_XBGR2101010:
>>>> - case DRM_FORMAT_ARGB2101010:
>>>> - case DRM_FORMAT_ABGR2101010:
>>>> - case DRM_FORMAT_XVYU2101010:
>>>> - case DRM_FORMAT_Y212:
>>>> - case DRM_FORMAT_Y216:
>>>> - return 6;
>>>> - case DRM_FORMAT_NV12:
>>>> - return 20;
>>>> - case DRM_FORMAT_P010:
>>>> - case DRM_FORMAT_P012:
>>>> - case DRM_FORMAT_P016:
>>>> - return 12;
>>>> - case DRM_FORMAT_XRGB16161616F:
>>>> - case DRM_FORMAT_XBGR16161616F:
>>>> - case DRM_FORMAT_ARGB16161616F:
>>>> - case DRM_FORMAT_ABGR16161616F:
>>>> - case DRM_FORMAT_XVYU12_16161616:
>>>> - case DRM_FORMAT_XVYU16161616:
>>>> - return 4;
>>>> - default:
>>>> - return 1;
>>>> - }
>>>> -}
>>>> -
>>>> -static int icl_max_plane_width(const struct drm_framebuffer *fb,
>>>> - int color_plane,
>>>> - unsigned int rotation)
>>>> -{
>>>> - return 5120;
>>>> -}
>>>> -
>>>> -static int skl_max_plane_height(void)
>>>> -{
>>>> - return 4096;
>>>> -}
>>>> -
>>>> -static int icl_max_plane_height(void)
>>>> -{
>>>> - return 4320;
>>>> -}
>>>>
>>>> static bool
>>>> skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state,
>>>> @@ -3874,35 +3753,55 @@ intel_plane_fence_y_offset(const struct intel_plane_state *plane_state)
>>>> return y;
>>>> }
>>>>
>>>> +static int intel_plane_min_width(struct intel_plane *plane,
>>>> + const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + if (plane->min_width)
>>>> + return plane->min_width(fb, color_plane, rotation);
>>>> + else
>>>> + return 1;
>>>> +}
>>>> +
>>>> +static int intel_plane_max_width(struct intel_plane *plane,
>>>> + const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + if (plane->max_width)
>>>> + return plane->max_width(fb, color_plane, rotation);
>>>> + else
>>>> + return INT_MAX;
>>>> +}
>>>> +
>>>> +static int intel_plane_max_height(struct intel_plane *plane,
>>>> + const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + if (plane->max_height)
>>>> + return plane->max_height(fb, color_plane, rotation);
>>>> + else
>>>> + return INT_MAX;
>>>> +}
>>>> +
>>>> static int skl_check_main_surface(struct intel_plane_state *plane_state)
>>>> {
>>>> - struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev);
>>>> + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>>>> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>>> const struct drm_framebuffer *fb = plane_state->hw.fb;
>>>> unsigned int rotation = plane_state->hw.rotation;
>>>> int x = plane_state->uapi.src.x1 >> 16;
>>>> int y = plane_state->uapi.src.y1 >> 16;
>>>> int w = drm_rect_width(&plane_state->uapi.src) >> 16;
>>>> int h = drm_rect_height(&plane_state->uapi.src) >> 16;
>>>> - int max_width, min_width, max_height;
>>>> - u32 alignment, offset;
>>>> + int min_width = intel_plane_min_width(plane, fb, 0, rotation);
>>>> + int max_width = intel_plane_max_width(plane, fb, 0, rotation);
>>>> + int max_height = intel_plane_max_height(plane, fb, 0, rotation);
>>>> int aux_plane = intel_main_to_aux_plane(fb, 0);
>>>> u32 aux_offset = plane_state->color_plane[aux_plane].offset;
>>>> -
>>>> - if (INTEL_GEN(dev_priv) >= 11) {
>>>> - max_width = icl_max_plane_width(fb, 0, rotation);
>>>> - min_width = icl_min_plane_width(fb);
>>>> - } else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
>>>> - max_width = glk_max_plane_width(fb, 0, rotation);
>>>> - min_width = 1;
>>>> - } else {
>>>> - max_width = skl_max_plane_width(fb, 0, rotation);
>>>> - min_width = 1;
>>>> - }
>>>> -
>>>> - if (INTEL_GEN(dev_priv) >= 11)
>>>> - max_height = icl_max_plane_height();
>>>> - else
>>>> - max_height = skl_max_plane_height();
>>>> + u32 alignment, offset;
>>>>
>>>> if (w > max_width || w < min_width || h > max_height) {
>>>> drm_dbg_kms(&dev_priv->drm,
>>>> @@ -3985,22 +3884,19 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
>>>>
>>>> static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
>>>> {
>>>> - struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
>>>> + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>>>> + struct drm_i915_private *i915 = to_i915(plane->base.dev);
>>>> const struct drm_framebuffer *fb = plane_state->hw.fb;
>>>> unsigned int rotation = plane_state->hw.rotation;
>>>> int uv_plane = 1;
>>>> - int max_width = skl_max_plane_width(fb, uv_plane, rotation);
>>>> - int max_height = 4096;
>>>> + int max_width = intel_plane_max_width(plane, fb, uv_plane, rotation);
>>>> + int max_height = intel_plane_max_height(plane, fb, uv_plane, rotation);
>>>> int x = plane_state->uapi.src.x1 >> 17;
>>>> int y = plane_state->uapi.src.y1 >> 17;
>>>> int w = drm_rect_width(&plane_state->uapi.src) >> 17;
>>>> int h = drm_rect_height(&plane_state->uapi.src) >> 17;
>>>> u32 offset;
>>>>
>>>> - intel_add_fb_offsets(&x, &y, plane_state, uv_plane);
>>>> - offset = intel_plane_compute_aligned_offset(&x, &y,
>>>> - plane_state, uv_plane);
>>>> -
>>>> /* FIXME not quite sure how/if these apply to the chroma plane */
>>>> if (w > max_width || h > max_height) {
>>>> drm_dbg_kms(&i915->drm,
>>>> @@ -4009,6 +3905,10 @@ static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
>>>> return -EINVAL;
>>>> }
>>>>
>>>> + intel_add_fb_offsets(&x, &y, plane_state, uv_plane);
>>>> + offset = intel_plane_compute_aligned_offset(&x, &y,
>>>> + plane_state, uv_plane);
>>>> +
>>>> if (is_ccs_modifier(fb->modifier)) {
>>>> int ccs_plane = main_to_ccs_plane(fb, uv_plane);
>>>> int aux_offset = plane_state->color_plane[ccs_plane].offset;
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>>>> index 3d4bf9b6a0a2..a16120508318 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>>>> @@ -1170,6 +1170,15 @@ struct intel_plane {
>>>> * the intel_plane_state structure and accessed via plane_state.
>>>> */
>>>>
>>>> + int (*min_width)(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation);
>>>> + int (*max_width)(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation);
>>>> + int (*max_height)(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation);
>>>> unsigned int (*max_stride)(struct intel_plane *plane,
>>>> u32 pixel_format, u64 modifier,
>>>> unsigned int rotation);
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
>>>> index 63040cb0d4e1..d682ab1c0f70 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
>>>> @@ -393,6 +393,134 @@ static int skl_plane_min_cdclk(const struct intel_crtc_state *crtc_state,
>>>> return DIV_ROUND_UP(pixel_rate * num, den);
>>>> }
>>>>
>>>> +static int skl_plane_max_width(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + int cpp = fb->format->cpp[color_plane];
>>>> +
>>>> + switch (fb->modifier) {
>>>> + case DRM_FORMAT_MOD_LINEAR:
>>>> + case I915_FORMAT_MOD_X_TILED:
>>>> + /*
>>>> + * Validated limit is 4k, but has 5k should
>>>> + * work apart from the following features:
>>>> + * - Ytile (already limited to 4k)
>>>> + * - FP16 (already limited to 4k)
>>>> + * - render compression (already limited to 4k)
>>>> + * - KVMR sprite and cursor (don't care)
>>>> + * - horizontal panning (TODO verify this)
>>>> + * - pipe and plane scaling (TODO verify this)
>>>> + */
>>>> + if (cpp == 8)
>>>> + return 4096;
>>>> + else
>>>> + return 5120;
>>>> + case I915_FORMAT_MOD_Y_TILED_CCS:
>>>> + case I915_FORMAT_MOD_Yf_TILED_CCS:
>>>> + case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
>>>> + /* FIXME AUX plane? */
>>>> + case I915_FORMAT_MOD_Y_TILED:
>>>> + case I915_FORMAT_MOD_Yf_TILED:
>>>> + if (cpp == 8)
>>>> + return 2048;
>>>> + else
>>>> + return 4096;
>>>> + default:
>>>> + MISSING_CASE(fb->modifier);
>>>> + return 2048;
>>>> + }
>>>> +}
>>>> +
>>>> +static int glk_plane_max_width(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + int cpp = fb->format->cpp[color_plane];
>>>> +
>>>> + switch (fb->modifier) {
>>>> + case DRM_FORMAT_MOD_LINEAR:
>>>> + case I915_FORMAT_MOD_X_TILED:
>>>> + if (cpp == 8)
>>>> + return 4096;
>>>> + else
>>>> + return 5120;
>>>> + case I915_FORMAT_MOD_Y_TILED_CCS:
>>>> + case I915_FORMAT_MOD_Yf_TILED_CCS:
>>>> + /* FIXME AUX plane? */
>>>> + case I915_FORMAT_MOD_Y_TILED:
>>>> + case I915_FORMAT_MOD_Yf_TILED:
>>>> + if (cpp == 8)
>>>> + return 2048;
>>>> + else
>>>> + return 5120;
>>>> + default:
>>>> + MISSING_CASE(fb->modifier);
>>>> + return 2048;
>>>> + }
>>>> +}
>>>> +
>>>> +static int icl_plane_min_width(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + /* Wa_14011264657, Wa_14011050563: gen11+ */
>>>> + switch (fb->format->format) {
>>>> + case DRM_FORMAT_C8:
>>>> + return 18;
>>>> + case DRM_FORMAT_RGB565:
>>>> + return 10;
>>>> + case DRM_FORMAT_XRGB8888:
>>>> + case DRM_FORMAT_XBGR8888:
>>>> + case DRM_FORMAT_ARGB8888:
>>>> + case DRM_FORMAT_ABGR8888:
>>>> + case DRM_FORMAT_XRGB2101010:
>>>> + case DRM_FORMAT_XBGR2101010:
>>>> + case DRM_FORMAT_ARGB2101010:
>>>> + case DRM_FORMAT_ABGR2101010:
>>>> + case DRM_FORMAT_XVYU2101010:
>>>> + case DRM_FORMAT_Y212:
>>>> + case DRM_FORMAT_Y216:
>>>> + return 6;
>>>> + case DRM_FORMAT_NV12:
>>>> + return 20;
>>>> + case DRM_FORMAT_P010:
>>>> + case DRM_FORMAT_P012:
>>>> + case DRM_FORMAT_P016:
>>>> + return 12;
>>>> + case DRM_FORMAT_XRGB16161616F:
>>>> + case DRM_FORMAT_XBGR16161616F:
>>>> + case DRM_FORMAT_ARGB16161616F:
>>>> + case DRM_FORMAT_ABGR16161616F:
>>>> + case DRM_FORMAT_XVYU12_16161616:
>>>> + case DRM_FORMAT_XVYU16161616:
>>>> + return 4;
>>>> + default:
>>>> + return 1;
>>>> + }
>>>> +}
>>>> +
>>>> +static int icl_plane_max_width(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + return 5120;
>>>> +}
>>>> +
>>>> +static int skl_plane_max_height(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + return 4096;
>>>> +}
>>>> +
>>>> +static int icl_plane_max_height(const struct drm_framebuffer *fb,
>>>> + int color_plane,
>>>> + unsigned int rotation)
>>>> +{
>>>> + return 4320;
>>>> +}
>>>> +
>>>> static unsigned int
>>>> skl_plane_max_stride(struct intel_plane *plane,
>>>> u32 pixel_format, u64 modifier,
>>>> @@ -3083,6 +3211,18 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>>>> fbc->possible_framebuffer_bits |= plane->frontbuffer_bit;
>>>> }
>>>>
>>>> + if (INTEL_GEN(dev_priv) >= 11) {
>>>> + plane->min_width = icl_plane_min_width;
>>>> + plane->max_width = icl_plane_max_width;
>>>> + plane->max_height = icl_plane_max_height;
>>>> + } else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
>>>> + plane->max_width = glk_plane_max_width;
>>>> + plane->max_height = skl_plane_max_height;
>>>> + } else {
>>>> + plane->max_width = skl_plane_max_width;
>>>> + plane->max_height = skl_plane_max_height;
>>>> + }
>>>> +
>>>> plane->max_stride = skl_plane_max_stride;
>>>> plane->update_plane = skl_update_plane;
>>>> plane->disable_plane = skl_disable_plane;
>>>>
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>
More information about the Intel-gfx
mailing list