> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > Sent: Wednesday, March 14, 2018 9:06 PM > To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, > Maarten <maarten.lankhorst@xxxxxxxxx> > Subject: Re: [PATCH v13 12/17] drm/i915: Upscale scaler max scale > for NV12 > > 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? > It is supported. Thank you. As suggested, will check both min width and height for NV12 as > 16 Regards Vidya > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
