> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > Sent: Thursday, March 15, 2018 12:34 AM > To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; 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 > > 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.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? > > 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 Yeah :) Thank you. Will make the change and float the patch. Regards Vidya _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
