Op 14-03-18 om 18:08 schreef Ville Syrjälä: > On Wed, Mar 14, 2018 at 04:55:08PM +0100, Maarten Lankhorst wrote: >> 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.lankho...@linux.intel.com] >>>>> Sent: Wednesday, March 14, 2018 4:03 PM >>>>> To: Srinivas, Vidya <vidya.srini...@intel.com>; intel- >>>>> g...@lists.freedesktop.org >>>>> Cc: Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst, Maarten >>>>> <maarten.lankho...@intel.com> >>>>> Subject: Re: [Intel-gfx] [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.lankho...@linux.intel.com] >>>>>>> Sent: Wednesday, March 14, 2018 3:55 PM >>>>>>> To: Srinivas, Vidya <vidya.srini...@intel.com>; intel- >>>>>>> g...@lists.freedesktop.org >>>>>>> Cc: Syrjala, Ville <ville.syrj...@intel.com>; Lankhorst, Maarten >>>>>>> <maarten.lankho...@intel.com> >>>>>>> Subject: Re: [Intel-gfx] [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.kond...@intel.com> >>>>>>>>> >>>>>>>>> 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.tay...@intel.com> >>>>>>>>> Reviewed-by: Clinton Taylor <clinton.a.tay...@intel.com> >>>>>>>>> Signed-off-by: Chandra Konduru <chandra.kond...@intel.com> >>>>>>>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.ma...@intel.com> >>>>>>>>> Signed-off-by: Uma Shankar <uma.shan...@intel.com> >>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srini...@intel.com> >>>>>>>>> --- >>>>>>>>> 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? > Possibly. I think the scaler should be operating on the rotated data, so from > the scaler POV we probably need to consider the rotated coordinates when > figuring out the width vs. height limits. But I didn't actually check > the code which way we program the scaler input size. > There's not much of a usecase for a 8x16 fb though, might as well limit it to 16x16. :)
~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
