[Intel-gfx] [PATCH] [RFC] drm/i915/skl+: enable PF_ID interlace mode in SKL
Mahesh Kumar
mahesh1.kumar at intel.com
Tue Jun 20 07:45:37 UTC 2017
Hi,
On Monday 19 June 2017 06:35 PM, Ville Syrjälä wrote:
> 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 at intel.com>
>>>> 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.
got it..
>> 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.
yes, this is the same behavior as per my experiment, Hardware
automatically taking care of everything.
(it's not reducing programmed destination size by 2).
I'm just setting source-size == destination-size & src-pos == dst-pos
for scaler & hardware automagically taking care of all the conversions.
>
> 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.
Agree, setting this in ilk_pipe_pixel_rate() will be right place, will
incorporate this :)
>
> 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.
yeah, PF-ID is needed only if user want Y-tiled buffer and/or 90/270
rotation.
But If we add a new property to choose mode (PF-PD / PF-ID / IF-ID) who
will be the user for this connector property?
We don't have any open-source user available to use this property as of now.
Please suggest if that is ok to use?
-Mahesh
>> 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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list