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@xxxxxxxxxxxxxxx> >>>> >>>> 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@xxxxxxxxxxxxxxx> >>> LGTM.. >>> Reviewed-by: Aditya Swarup <aditya.swarup@xxxxxxxxx> >> 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@xxxxxxxxxxxxxxxxxxxxx >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
