On Mon, Jun 19, 2017 at 05:50:28PM +0530, Mahesh Kumar wrote: > Hi Ville, > > Thanks for review > > > On Friday 16 June 2017 07:47 PM, Ville Syrjälä wrote: > > On Wed, Jun 07, 2017 at 03:19:05PM +0530, Mahesh Kumar wrote: > >> In previous GEN default Interlace mode enabled is IF-ID mode, but IF-ID > >> mode has many limitations in SKL. This mode doesn't support y-tiling, > >> 90-270 rotation is not supported & YUV-420 planar source pixel formats > >> are not supported with above mode. > > I think we'll want something much more simple for stable. And that > > should probably be just -EINVAL if we try to do any of those > > forbidden things with an interlaced mode. > Agree. > > > >> This patch make changes to use PF-ID Interlace mode in SKL+ platforms. > >> PF-ID mode require one scaler to be attached in pipe-scaling mode. > >> During WM calculation adjusted pixel rate need to be doubled. > >> During max_supported_pixel_format calculation vertical downscale is 2.0. > >> > >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90238 > >> --- > >> drivers/gpu/drm/i915/intel_atomic.c | 13 ++++++++++ > >> drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++++++----- > >> drivers/gpu/drm/i915/intel_drv.h | 10 ++++++++ > >> drivers/gpu/drm/i915/intel_panel.c | 9 +++++-- > >> drivers/gpu/drm/i915/intel_pm.c | 17 ++++++++++++- > >> 5 files changed, 87 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > >> index d791b3ef89b5..06ed2fc449d7 100644 > >> --- a/drivers/gpu/drm/i915/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/intel_atomic.c > >> @@ -248,6 +248,13 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, > >> return -EINVAL; > >> } > >> > >> + if (num_scalers_need > 1 && > >> + scaler_state->scaler_users & (1 << SKL_CRTC_INDEX) && > >> + scaler_state->scaler_users & (1 << SKL_PF_ID_INDEX)) { > >> + DRM_ERROR("Can't attach scaler to CRTC in Interlace Mode\n"); > >> + return -EINVAL; > >> + } > > I thought the whole point of PF-ID here was that we can then do pipe > > scaling (and other things). So this check looks wrong to me. > With PF-ID we can have Y-tiled buffers, & 90/270 rotation enabled, It > doesn't explicitly talk about scaling. PF-ID itself is scaling. Presumably you don't have to have exactly 2:1 scaling factor with PF-ID. > BSpec doesn't limit us from using 2 scalers as a pipe scaler, still I > sent a query to HW team regarding same. We don't want to use multiple scalers. I dubt that would even work. And before SKL the panel fitter was a dedicated piece of hw anyway so there was no way to even try to assign multiple scalers to the pipe. > > Current S/W framework doesn't allows us to attach multiple pipe-scalers > in a pipe (multiple scaler_id associated with one crtc_state). that's > the reason for above check. > > > > And I'm thinking we shouldn't be adding yet another scaler user for > > this, and instead it should just be part of the normal pfit setup. > To use existing scaler framework, we need one scaler-user to be > associated with it. > If we use existing CRTC_INDEX user then there will be many corner cases > to handle. I'm not 100% whether we need any adjustment in the pfit code itself. The spec doesn't seem to tell us that we need to adjust the panel fitter window in any way. That's a little inconsistent with the resulting scaling factor, but I guess the hw will automagically reduce the destination size by two when the pipe is in the PF-ID mode. So I think it should mostly be a simple matter of calling the pfit code to set up the pos/size, and then you should just adjust ilk_pipe_pixel_rate() to double the resulting pixel rate. That said, I'm not really sure we want to flip PF-ID on automagically. It would result in scaling when the user doesn't necessarily want it. So we might want to think about some kind if property to control this stuff. > To make it simple I added new scaler-user. > This code uses pfit setup only to attach/enable Pipe-scaler. > Please let me know if I didn't understood your concern correctly. > Any other suggestions/queries are welcome. > > -Mahesh > > > > >> + > >> /* walkthrough scaler_users bits and start assigning scalers */ > >> for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) { > >> int *scaler_id; > >> @@ -264,6 +271,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, > >> > >> /* panel fitter case: assign as a crtc scaler */ > >> scaler_id = &scaler_state->scaler_id; > >> + } else if (i == SKL_PF_ID_INDEX) { > >> + name = "PF-ID INTERLACE MODE"; > >> + idx = intel_crtc->base.base.id; > >> + > >> + /* PF-ID interlaced mode case: needs a pipe scaler */ > >> + scaler_id = &scaler_state->scaler_id; > >> } else { > >> name = "PLANE"; > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 85ac32549e85..15c79b46d645 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > >> */ > >> need_scaling = src_w != dst_w || src_h != dst_h; > >> > >> + if ((crtc_state->base.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) > >> + && scaler_user == SKL_PF_ID_INDEX) > >> + /* PF-ID Interlace mode needs a scaler */ > >> + need_scaling = true; > >> + > >> /* > >> * if plane is being disabled or scaler is no more required or force detach > >> * - free scaler binded to this plane/crtc > >> @@ -4635,16 +4640,24 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > >> * scaler can be assigned to other user. Actual register > >> * update to free the scaler is done in plane/panel-fit programming. > >> * For this purpose crtc/plane_state->scaler_id isn't reset here. > >> + * Now Interlace mode is a new user of pipe-scaler, so free the scaler-id > >> + * only if call is for respective user. If no user for scaler free it. > >> */ > >> if (force_detach || !need_scaling) { > >> - if (*scaler_id >= 0) { > >> + if (*scaler_id >= 0 && > >> + scaler_state->scaler_users & (1 << scaler_user)) { > >> scaler_state->scaler_users &= ~(1 << scaler_user); > >> scaler_state->scalers[*scaler_id].in_use = 0; > >> > >> DRM_DEBUG_KMS("scaler_user index %u.%u: " > >> "Staged freeing scaler id %d scaler_users = 0x%x\n", > >> - intel_crtc->pipe, scaler_user, *scaler_id, > >> - scaler_state->scaler_users); > >> + intel_crtc->pipe, scaler_user, > >> + *scaler_id, scaler_state->scaler_users); > >> + > >> + *scaler_id = -1; > >> + } > >> + if (*scaler_id >= 0 && !scaler_state->scaler_users) { > >> + scaler_state->scalers[*scaler_id].in_use = 0; > >> *scaler_id = -1; > >> } > >> return 0; > >> @@ -4691,6 +4704,16 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state) > >> adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay); > >> } > >> > >> +static int skl_update_scaler_crtc_pf_id_output(struct intel_crtc_state *state) > >> +{ > >> + const struct drm_display_mode *mode = &state->base.adjusted_mode; > >> + > >> + return skl_update_scaler(state, !state->base.active, > >> + SKL_PF_ID_INDEX, &state->scaler_state.scaler_id, > >> + state->pipe_src_w, state->pipe_src_h, > >> + mode->crtc_hdisplay, mode->crtc_vdisplay); > >> +} > >> + > >> /** > >> * skl_update_scaler_plane - Stages update to scaler state for a given plane. > >> * > >> @@ -6258,6 +6281,15 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > >> } > >> } > >> > >> + if (INTEL_GEN(dev_priv) >= 9) { > >> + if (skl_update_scaler_crtc_pf_id_output(pipe_config)) { > >> + DRM_ERROR("Unable to set scaler for PF-ID mode\n"); > >> + return -EINVAL; > >> + } > >> + intel_pch_panel_fitting(crtc, pipe_config, > >> + DRM_MODE_SCALE_FULLSCREEN); > >> + } > >> + > >> if (adjusted_mode->crtc_clock > clock_limit) { > >> DRM_DEBUG_KMS("requested pixel clock (%d kHz) too high (max: %d kHz, double wide: %s)\n", > >> adjusted_mode->crtc_clock, clock_limit, > >> @@ -8045,9 +8077,12 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc) > >> if (IS_HASWELL(dev_priv) && intel_crtc->config->dither) > >> val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP); > >> > >> - if (intel_crtc->config->base.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) > >> - val |= PIPECONF_INTERLACED_ILK; > >> - else > >> + if (intel_crtc->config->base.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) { > >> + if (INTEL_GEN(dev_priv) >= 9) > >> + val |= PIPECONF_PFIT_PF_INTERLACED_ILK; > >> + else > >> + val |= PIPECONF_INTERLACED_ILK; > >> + } else > >> val |= PIPECONF_PROGRESSIVE; > >> > >> I915_WRITE(PIPECONF(cpu_transcoder), val); > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index 83dd40905821..d4546f6498b9 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -483,6 +483,7 @@ struct intel_crtc_scaler_state { > >> * avilability. > >> */ > >> #define SKL_CRTC_INDEX 31 > >> +#define SKL_PF_ID_INDEX 29 > >> unsigned scaler_users; > >> > >> /* scaler used by crtc for panel fitting purpose */ > >> @@ -1206,6 +1207,15 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi) > >> return container_of(intel_hdmi, struct intel_digital_port, hdmi); > >> } > >> > >> +static inline bool > >> +is_skl_interlace_mode(struct drm_i915_private *dev_priv, > >> + const struct drm_display_mode *mode) > >> +{ > >> + if (INTEL_GEN(dev_priv) < 9) > >> + return false; > >> + return mode->flags & DRM_MODE_FLAG_INTERLACE; > >> +} > >> + > >> /* intel_fifo_underrun.c */ > >> bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv, > >> enum pipe pipe, bool enable); > >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > >> index 4114cb3f14e7..fae7fe7802f8 100644 > >> --- a/drivers/gpu/drm/i915/intel_panel.c > >> +++ b/drivers/gpu/drm/i915/intel_panel.c > >> @@ -108,9 +108,14 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, > >> const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; > >> int x = 0, y = 0, width = 0, height = 0; > >> > >> - /* Native modes don't need fitting */ > >> + /* > >> + * Native modes don't need fitting > >> + * PF-ID Interlace mode in SKL+ require a pipe scaler > >> + */ > >> if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && > >> - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) > >> + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && > >> + !(is_skl_interlace_mode(to_i915(intel_crtc->base.dev), > >> + adjusted_mode))) > >> goto done; > >> > >> switch (fitting_mode) { > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >> index aa9d8cef7ce0..dfb21f00fbed 100644 > >> --- a/drivers/gpu/drm/i915/intel_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_pm.c > >> @@ -3873,15 +3873,18 @@ static uint_fixed_16_16_t > >> skl_pipe_downscale_amount(const struct intel_crtc_state *crtc_state) > >> { > >> uint_fixed_16_16_t pipe_downscale = u32_to_fixed_16_16(1); > >> + const struct drm_display_mode *mode; > >> > >> if (!crtc_state->base.enable) > >> return pipe_downscale; > >> > >> + mode = &crtc_state->base.adjusted_mode; > >> if (crtc_state->pch_pfit.enabled) { > >> uint32_t src_w, src_h, dst_w, dst_h; > >> uint32_t pfit_size = crtc_state->pch_pfit.size; > >> uint_fixed_16_16_t fp_w_ratio, fp_h_ratio; > >> uint_fixed_16_16_t downscale_h, downscale_w; > >> + uint_fixed_16_16_t min_h_downscale; > >> > >> src_w = crtc_state->pipe_src_w; > >> src_h = crtc_state->pipe_src_h; > >> @@ -3891,10 +3894,19 @@ skl_pipe_downscale_amount(const struct intel_crtc_state *crtc_state) > >> if (!dst_w || !dst_h) > >> return pipe_downscale; > >> > >> + /* > >> + * Interlace mode require 1 scaler so no other scaler can be > >> + * attached to pipe. PF-ID mode is equivalent to 2.0 vertical > >> + * downscale. > >> + */ > >> + if (mode->flags & DRM_MODE_FLAG_INTERLACE) > >> + min_h_downscale = u32_to_fixed_16_16(2); > >> + else > >> + min_h_downscale = u32_to_fixed_16_16(1); > >> fp_w_ratio = fixed_16_16_div(src_w, dst_w); > >> fp_h_ratio = fixed_16_16_div(src_h, dst_h); > >> downscale_w = max_fixed_16_16(fp_w_ratio, u32_to_fixed_16_16(1)); > >> - downscale_h = max_fixed_16_16(fp_h_ratio, u32_to_fixed_16_16(1)); > >> + downscale_h = max_fixed_16_16(fp_h_ratio, min_h_downscale); > >> > >> pipe_downscale = mul_fixed16(downscale_w, downscale_h); > >> } > >> @@ -4418,6 +4430,9 @@ skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cstate, > >> * with additional adjustments for plane-specific scaling. > >> */ > >> adjusted_pixel_rate = cstate->pixel_rate; > >> + if (cstate->base.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) > >> + adjusted_pixel_rate *= 2; > >> + > >> downscale_amount = skl_plane_downscale_amount(cstate, pstate); > >> > >> return mul_round_up_u32_fixed16(adjusted_pixel_rate, > >> -- > >> 2.11.0 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
