Op 14-03-18 om 16:35 schreef Ville Syrjälä: > On Wed, Mar 14, 2018 at 10:36:32AM +0000, Srinivas, Vidya wrote: >> >>> -----Original Message----- >>> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] >>> Sent: Wednesday, March 14, 2018 4:03 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 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.. >> As such there is no restriction on width for YUV in Bspec. It only mentions >> about the height. > Do remember to consider 90/270 degree rotation. That's still supported > with NV12 is it not? > Should we also force a minimum width of 16 then? Minimum width of 8 was based on SKL_MIN_SRC_W, since scaler is always enabled for NV12 that ends up being a restriction for the format. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
