Re: [PATCH 12/16] drm/i915: Upscale scaler max scale for NV12

From: "Srinivas, Vidya" <vidya.srinivas@intel.com>
To: "Sharma, Shashank" <shashank.sharma@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 12/16] drm/i915: Upscale scaler max scale for NV12
Date: Mon, 12 Feb 2018 08:31:48 +0000	[thread overview]
Message-ID: <F653A0A18852B74D88578FA2EB7094EAB682304D@BGSMSX108.gar.corp.intel.com> (raw)
In-Reply-To: <19a3ff3e-49af-d158-e68b-58907e6a088a@intel.com>


> -----Original Message-----
> From: Sharma, Shashank
> Sent: Thursday, February 8, 2018 3:16 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: maarten.lankhorst@linux.intel.com; Kamath, Sunil
> <sunil.kamath@intel.com>; Shankar, Uma <uma.shankar@intel.com>;
> Konduru, Chandra <chandra.konduru@intel.com>; Maiti, Nabendu Bikash
> <nabendu.bikash.maiti@intel.com>
> Subject: Re: [PATCH 12/16] drm/i915: Upscale scaler max scale for NV12
> 
> Regards
> 
> Shashank
> 
> 
> On 2/6/2018 6:28 PM, Vidya Srinivas wrote:
> > From: Chandra Konduru <chandra.konduru@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)
> >
> > Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
> > Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 33 +++++++++++++++++++++++---
> -------
> >   drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> >   drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
> >   3 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 5fc9255..74757d0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3435,6 +3435,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);
> >   	}
> > @@ -4658,7 +4660,8 @@ 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,
> > +		  uint32_t pixel_format)
> >   {
> >   	struct intel_crtc_scaler_state *scaler_state =
> >   		&crtc_state->scaler_state;
> > @@ -4674,7 +4677,8 @@ skl_update_scaler(struct intel_crtc_state
> *crtc_state, bool force_detach,
> >   	 * the 90/270 degree plane rotation cases (to match the
> >   	 * GTT mapping), hence no need to account for rotation here.
> >   	 */
> > -	need_scaling = src_w != dst_w || src_h != dst_h;
> > +	need_scaling = src_w != dst_w || src_h != dst_h ||
> > +				   (pixel_format == DRM_FORMAT_NV12);
> This line needs to be aligned to above line, after = , also no need for braces
> >
> >   	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> >   		need_scaling = true;
> > @@ -4753,7 +4757,7 @@ int skl_update_scaler_crtc(struct
> intel_crtc_state *state)
> >   	return skl_update_scaler(state, !state->base.active,
> SKL_CRTC_INDEX,
> >   		&state->scaler_state.scaler_id,
> >   		state->pipe_src_w, state->pipe_src_h,
> > -		adjusted_mode->crtc_hdisplay, adjusted_mode-
> >crtc_vdisplay);
> > +		adjusted_mode->crtc_hdisplay, adjusted_mode-
> >crtc_vdisplay, 0);
> Why 0 ? shouldn't we send actual current pixel format ? Or if you are just
> looking for pixel format is NV12 or not, you can make this variable a bool
> and name it is_nv12 or is_yuv420
> >   }
Thank you. We don't check for pixel format for update scaler during crtc check..
We need it only for plane check. we don't want to derive the fb and its format
here. So 0 was passed. Will see if I can modify to make more sense here.
> >
> >   /**
> > @@ -4783,7 +4787,8 @@ static int skl_update_scaler_plane(struct
> intel_crtc_state *crtc_state,
> >   				drm_rect_width(&plane_state->base.src) >>
> 16,
> >   				drm_rect_height(&plane_state->base.src) >>
> 16,
> >   				drm_rect_width(&plane_state->base.dst),
> > -				drm_rect_height(&plane_state->base.dst));
> > +				drm_rect_height(&plane_state->base.dst),
> > +				fb ? fb->format->format : 0);
> Again, sending 0 doesn't look logical to me
Thank you. Agree, will change this.
> >
> >   	if (ret || plane_state->scaler_id < 0)
> >   		return ret;
> > @@ -4809,6 +4814,7 @@ static int skl_update_scaler_plane(struct
> intel_crtc_state *crtc_state,
> >   	case DRM_FORMAT_YVYU:
> >   	case DRM_FORMAT_UYVY:
> >   	case DRM_FORMAT_VYUY:
> > +	case DRM_FORMAT_NV12:
> >   		break;
> >   	default:
> >   		DRM_DEBUG_KMS("[PLANE:%d:%s] FB:%d unsupported
> scaling format
> > 0x%x\n", @@ -12758,11 +12764,12 @@ intel_cleanup_plane_fb(struct
> drm_plane *plane,
> >   }
> >
> >   int
> > -skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state
> > *crtc_state)
> > +skl_max_scale(struct intel_crtc *intel_crtc,
> > +	struct intel_crtc_state *crtc_state, uint32_t pixel_format)
> this line should be aligned to the '(' from above line
> >   {
> >   	struct drm_i915_private *dev_priv;
> > -	int max_scale;
> > -	int crtc_clock, max_dotclk;
> > +	int max_scale, mult;
> > +	int crtc_clock, max_dotclk, tmpclk1, tmpclk2;
> >
> >   	if (!intel_crtc || !crtc_state->base.enable)
> >   		return DRM_PLANE_HELPER_NO_SCALING; @@ -12784,8
> +12791,10 @@
> > skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state
> *crtc_state
> >   	 *            or
> >   	 *    cdclk/crtc_clock
> >   	 */
> > -	max_scale = min((1 << 16) * 3 - 1,
> > -			(1 << 8) * ((max_dotclk << 8) / crtc_clock));
> > +	mult = pixel_format == DRM_FORMAT_NV12 ? 2 : 3;
> > +	tmpclk1 = (1 << 16) * mult - 1;
> > +	tmpclk2 = (1 << 8) * ((max_dotclk << 8) / crtc_clock);
> > +	max_scale = min(tmpclk1, tmpclk2);
> >
> >   	return max_scale;
> >   }
> > @@ -12807,7 +12816,11 @@ intel_check_primary_plane(struct
> intel_plane *plane,
> >   		/* use scaler when colorkey is not required */
> >   		if (!state->ckey.flags) {
> >   			min_scale = 1;
> > -			max_scale = skl_max_scale(to_intel_crtc(crtc),
> crtc_state);
> > +			max_scale = skl_max_scale(to_intel_crtc(crtc),
> > +						crtc_state,
> > +						state->base.fb ?
> > +						state->base.fb->format-
> >format :
> > +						0);
> All these parameters should be aligned to the first opening brace '('
> >   		}
> >   		can_position = true;
> >   	}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d4c027a..111f1a4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1580,7 +1580,8 @@ void intel_mode_from_pipe_config(struct
> drm_display_mode *mode,
> >   				 struct intel_crtc_state *pipe_config);
> >
> >   int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
> > -int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state
> > *crtc_state);
> > +int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state
> *crtc_state,
> > +	uint32_t pixel_format);
> Align to above '('
> >
> >   static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state
> *state)
> >   {
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 9e31be2..f2e144b 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -897,7 +897,8 @@ intel_check_sprite_plane(struct intel_plane
> *plane,
> >   		if (!state->ckey.flags) {
> >   			can_scale = 1;
> >   			min_scale = 1;
> > -			max_scale = skl_max_scale(crtc, crtc_state);
> > +			max_scale = skl_max_scale(crtc, crtc_state,
> > +						fb->format->format);
> Alignment
> >   		} else {
> >   			can_scale = 0;
> >   			min_scale = DRM_PLANE_HELPER_NO_SCALING;
> - Shashank
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx