Op 14-03-18 om 11:31 schreef Srinivas, Vidya: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] >> Sent: Wednesday, March 14, 2018 3:55 PM >> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel- >> gfx@xxxxxxxxxxxxxxxxxxxxx >> Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten >> <maarten.lankhorst@xxxxxxxxx> >> Subject: Re: [PATCH v13 12/17] drm/i915: Upscale scaler max scale >> for NV12 >> >> 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 > Thank you, I will make this change and float the patch. > > Regards > Vidya For the framebuffer creation also require minimum width then, since it needs to be SKL_MIN_SRC_W too.. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
