Op 14-03-18 om 10:52 schreef Maarten Lankhorst:
> Op 09-03-18 om 09:48 schreef Vidya Srinivas:
>> From: Chandra Konduru <chandra.konduru@xxxxxxxxx>
>>
>> This patch updates scaler max limit support for NV12
>>
>> v2: Rebased (me)
>>
>> v3: Rebased (me)
>>
>> v4: Missed the Tested-by/Reviewed-by in the previous series
>> Adding the same to commit message in this version.
>>
>> v5: Addressed review comments from Ville and rebased
>> - calculation of max_scale to be made
>> less convoluted by splitting it up a bit
>> - Indentation errors to be fixed in the series
>>
>> v6: Rebased (me)
>> Fixed review comments from Paauwe, Bob J
>> Previous version, where a split of calculation
>> was done, was wrong. Fixed that issue here.
>>
>> v7: Rebased (me)
>>
>> v8: Rebased (me)
>>
>> v9: Rebased (me)
>>
>> v10: Rebased (me)
>>
>> v11: Addressed review comments from Shashank Sharma
>> Alignment issues fixed.
>> When call to skl_update_scaler is made, 0 was being
>> sent instead of pixel_format.
>> When crtc update scaler is called, we dont have the
>> fb to derive the pixel format. Added the function
>> parameter bool plane_scaler_check to account for this.
>>
>> v12: Fixed failure in IGT debugfs_test.
>> fb is NULL in skl_update_scaler_plane
>> Due to this, accessing fb->format caused failure.
>> Patch checks fb before using.
>>
>> v13: In the previous version there was a flaw.
>> In skl_update_scaler during plane_scaler_check
>> if the format was non-NV12, it would set need_scaling
>> to false. This could reset the previously set need_scaling
>> from a previous condition check. Patch fixes this.
>> Patch also adds minimum src height for YUV 420 formats
>> to 16 (as defined in BSpec) and adds for checking this
>> range.
>>
>> Tested-by: Clinton Taylor <clinton.a.taylor@xxxxxxxxx>
>> Reviewed-by: Clinton Taylor <clinton.a.taylor@xxxxxxxxx>
>> Signed-off-by: Chandra Konduru <chandra.konduru@xxxxxxxxx>
>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@xxxxxxxxx>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>> Signed-off-by: Vidya Srinivas <vidya.srinivas@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 78 ++++++++++++++++++++++++++----------
>> drivers/gpu/drm/i915/intel_drv.h | 4 +-
>> drivers/gpu/drm/i915/intel_sprite.c | 3 +-
>> 3 files changed, 61 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 34f7225..7fd8354 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3466,6 +3466,8 @@ static u32 skl_plane_ctl_format(uint32_t pixel_format)
>> return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_UYVY;
>> case DRM_FORMAT_VYUY:
>> return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_VYUY;
>> + case DRM_FORMAT_NV12:
>> + return PLANE_CTL_FORMAT_NV12;
>> default:
>> MISSING_CASE(pixel_format);
>> }
>> @@ -4705,7 +4707,9 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>> static int
>> skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>> unsigned int scaler_user, int *scaler_id,
>> - int src_w, int src_h, int dst_w, int dst_h)
>> + int src_w, int src_h, int dst_w, int dst_h,
>> + bool plane_scaler_check,
>> + uint32_t pixel_format)
>> {
>> struct intel_crtc_scaler_state *scaler_state =
>> &crtc_state->scaler_state;
>> @@ -4723,6 +4727,10 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>> */
>> need_scaling = src_w != dst_w || src_h != dst_h;
>>
>> + if (plane_scaler_check)
>> + if (pixel_format == DRM_FORMAT_NV12)
>> + need_scaling = true;
> Seems redundant to add plane_scaler_check, if you can just check for scaler_user != SKL_CRTC_INDEX.
> But since pixel_format is always 0 for crtc index, you can just check pixel_format == DRM_FORMAT_NV12 directly..
>
>> if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>> need_scaling = true;
>>
>> @@ -4763,17 +4771,32 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>> }
>>
>> /* range checks */
>> - if (src_w < SKL_MIN_SRC_W || src_h < SKL_MIN_SRC_H ||
>> - dst_w < SKL_MIN_DST_W || dst_h < SKL_MIN_DST_H ||
>> -
>> - src_w > SKL_MAX_SRC_W || src_h > SKL_MAX_SRC_H ||
>> - dst_w > SKL_MAX_DST_W || dst_h > SKL_MAX_DST_H) {
>> - DRM_DEBUG_KMS("scaler_user index %u.%u: src %ux%u dst %ux%u "
>> - "size is out of scaler range\n",
>> - intel_crtc->pipe, scaler_user, src_w, src_h, dst_w, dst_h);
>> - return -EINVAL;
>> - }
>> -
>> + if (plane_scaler_check && pixel_format == DRM_FORMAT_NV12) {
>> + if (src_h > SKL_MIN_YUV_420_SRC_H)
>> + goto check_scaler_range;
>> + else
>> + goto failed_range;
>> + } else {
>> + if (src_h >= SKL_MIN_SRC_H)
>> + goto check_scaler_range;
>> + else
>> + goto failed_range;
>> + }
> Since nv12 always needs scaling, could we refuse to create NV12 fb's with height < 16 in intel_framebuffer_init?
Hm we should probably reject this in that place anyway, but since src_h >= SKL_MIN_YUV_420_SRC_H
implies src_h >= SKL_MIN_SRC_H we don't need special handling, and can just do if (pixel_format == NV12 && src_h >= 16) return -EINVAL;
and keep the existing checks.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
