Op 08-08-17 om 14:51 schreef Mahesh Kumar:
> Hi,
>> +
>> + struct {
>> + uint16_t plane;
> should this also be named as plane_wm, because it's again wm with SR.
> just a nitpick but not a stopper.
Ack, will change (and for the other patches in this series too).
>
>> @@ -591,6 +600,9 @@ struct intel_crtc_wm_state {
>> /* optimal watermarks */
>> struct g4x_wm_state optimal;
>> } g4x;
> new line please to match the coding style of function
+1
>> + if (!plane_state || !intel_wm_plane_visible(crtc_state, to_intel_plane_state(plane_state))) {
>> + wm_state->plane_wm = fifo_size - wm_info->guard_size;
>> + if (wm_state->plane_wm > (long)wm_info->max_wm)
>> + wm_state->plane_wm = wm_info->max_wm;
>> + } else if (HAS_FW_BLC(dev_priv)) {
> This part will be called only for (GEN > 2,) So we'll never be
> calculating wm for GEN2. but you are changing GEN2 wm also to 2 stages
> compute & update in intel_init_pm.
Well spotted, should be fixed in a bit.
>> + struct drm_framebuffer *fb = plane_state->fb;
>> + unsigned cpp = IS_GEN2(dev_priv) ? 4 : fb->format->cpp[0];
>> const struct drm_display_mode *adjusted_mode =
>> - &crtc->config->base.adjusted_mode;
>> - const struct drm_framebuffer *fb =
>> - crtc->base.primary->state->fb;
>> - int cpp;
>> + &crtc_state->base.adjusted_mode;
>> + unsigned active_crtcs;
>> + bool may_cxsr;
>>
>> - if (IS_GEN2(dev_priv))
>> - cpp = 4;
>> + if (state->modeset)
>> + active_crtcs = state->active_crtcs;
>> else
>> - cpp = fb->format->cpp[0];
>> + active_crtcs = dev_priv->active_crtcs;
>>
>> - planea_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
>> - wm_info, fifo_size, cpp,
>> - pessimal_latency_ns);
>> - enabled = crtc;
>> - } else {
>> - planea_wm = fifo_size - wm_info->guard_size;
>> - if (planea_wm > (long)wm_info->max_wm)
>> - planea_wm = wm_info->max_wm;
>> + may_cxsr = active_crtcs == drm_crtc_mask(&crtc->base);
> It would be good to have a comment stating if only one CRTC is enabled
> we can go to cxsr.
> we can use hweight32() function to check if only one CRTC is enabled
> (not mandatory).
Added the comment, I think this makes it more clear for now.
>> + DRM_DEBUG_KMS("self-refresh entries: %ld\n", entries);
>> +
>> + if (wm_info->fifo_size >= entries) {
> Earlier if (fifo_size < entries) we were assigning srwm = 1 & not
> disabling cxsr, Is there any specific Bspec documentation to not enable
> cxsr if fifo_size < entries ?
Not sure, but it felt like this was an overflow.. Maybe I can find something on bspec about it.
>> + wm_state->cxsr = true;
>> + wm_state->sr.plane = wm_info->fifo_size - entries;
>> + } else
>> + may_cxsr = false;
> may_cxsr is not used later, so no need to overwrite the value.
True, I dump wm_state->cxsr now.
>> - if (IS_GEN2(dev_priv))
>> - cpp = 4;
>> + if (plane == PLANE_A)
> here plane is of type intel_plane but you are comparing with enum plane,
> I think you meant plane->plane
Right, nicely caught! Compiler didn't warn because PLANE_A == 0, it did complain if I changed it to PLANE_B:
drivers/gpu/drm/i915/intel_pm.c: In function ‘i9xx_wm_get_hw_state’:
drivers/gpu/drm/i915/intel_pm.c:2351:13: error: comparison between pointer and integer [-Werror]
if (plane == PLANE_B)
>> + wm_state->plane_wm = planea_wm;
>> else
>> - cpp = fb->format->cpp[0];
>> + wm_state->plane_wm = planeb_wm;
>> +
>> + crtc->wm.active.i9xx = *wm_state;
>> + }
>> +}
>>
>> - planeb_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
>> - wm_info, fifo_size, cpp,
>> - pessimal_latency_ns);
>> +static void i9xx_update_wm(struct intel_crtc *crtc)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> + uint32_t fwater_lo;
>> + uint32_t fwater_hi;
>> + int cwm, srwm = -1;
>> + int planea_wm, planeb_wm;
>> + struct intel_crtc *enabled = NULL;
> enabled is used to check if cxsr can be enabled, IMHO it would be good
> to take a bool variable for same & replace use of enabled with
> respective intel_crtc variable.
> if it seems complex then this approach is also good with me.
Yeah agreed, it should at least look at wm_state->cxsr. I need to keep the enabled check
in case pipe A gets enabled after pipe B. In that case watermarks on A are updated before
B, and we should really kill CXSR already at that point..
I'll just add the cxsr check, that should do it. :)
>> +
>> + crtc->wm.active.i9xx = crtc->config->wm.i9xx.optimal;
>> +
>> + crtc = intel_get_crtc_for_plane(dev_priv, 0);
>> + planea_wm = crtc->wm.active.i9xx.plane_wm;
>> + if (intel_crtc_active(crtc))
>> + enabled = crtc;
>> +
>> + crtc = intel_get_crtc_for_plane(dev_priv, 1);
>> + if (intel_crtc_active(crtc)) {
>> if (enabled == NULL)
>> enabled = crtc;
>> else
>> enabled = NULL;
>> - } else {
>> - planeb_wm = fifo_size - wm_info->guard_size;
>> - if (planeb_wm > (long)wm_info->max_wm)
>> - planeb_wm = wm_info->max_wm;
>> }
>> + planeb_wm = crtc->wm.active.i9xx.plane_wm;
>>
>> DRM_DEBUG_KMS("FIFO watermarks - A: %d, B: %d\n", planea_wm, planeb_wm);
>>
>> - if (IS_I915GM(dev_priv) && enabled) {
>> - struct drm_i915_gem_object *obj;
>> -
>> - obj = intel_fb_obj(enabled->base.primary->state->fb);
>> -
>> - /* self-refresh seems busted with untiled */
>> - if (!i915_gem_object_is_tiled(obj))
>> - enabled = NULL;
>> - }
>> + if (!enabled->wm.active.i9xx.cxsr)
>> + enabled = NULL;
>>
>> /*
>> * Overlay gets an aggressive default since video jitter is bad.
>> @@ -2317,31 +2382,8 @@ static void i9xx_update_wm(struct intel_crtc *unused_crtc)
>> intel_set_memory_cxsr(dev_priv, false);
>>
>> /* Calc sr entries for one plane configs */
>> - if (HAS_FW_BLC(dev_priv) && enabled) {
>> - /* self-refresh has much higher latency */
>> - static const int sr_latency_ns = 6000;
>> - const struct drm_display_mode *adjusted_mode =
>> - &enabled->config->base.adjusted_mode;
>> - const struct drm_framebuffer *fb =
>> - enabled->base.primary->state->fb;
>> - int clock = adjusted_mode->crtc_clock;
>> - int htotal = adjusted_mode->crtc_htotal;
>> - int hdisplay = enabled->config->pipe_src_w;
>> - int cpp;
>> - int entries;
>> -
>> - if (IS_I915GM(dev_priv) || IS_I945GM(dev_priv))
>> - cpp = 4;
>> - else
>> - cpp = fb->format->cpp[0];
>> -
>> - entries = intel_wm_method2(clock, htotal, hdisplay, cpp,
>> - sr_latency_ns / 100);
>> - entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
>> - DRM_DEBUG_KMS("self-refresh entries: %d\n", entries);
>> - srwm = wm_info->fifo_size - entries;
>> - if (srwm < 0)
>> - srwm = 1;
>> + if (enabled) {
>> + srwm = enabled->wm.active.i9xx.sr.plane;
>>
>> if (IS_I945G(dev_priv) || IS_I945GM(dev_priv))
>> I915_WRITE(FW_BLC_SELF,
>> @@ -2367,23 +2409,18 @@ static void i9xx_update_wm(struct intel_crtc *unused_crtc)
>> intel_set_memory_cxsr(dev_priv, true);
>> }
>>
>> -static void i845_update_wm(struct intel_crtc *unused_crtc)
>> +static void i845_update_wm(struct intel_crtc *crtc)
>> {
>> - struct drm_i915_private *dev_priv = to_i915(unused_crtc->base.dev);
>> - struct intel_crtc *crtc;
>> - const struct drm_display_mode *adjusted_mode;
>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> uint32_t fwater_lo;
>> int planea_wm;
>>
>> - crtc = single_enabled_crtc(dev_priv);
>> - if (crtc == NULL)
> earlier we were checking if enabled crtc's != 1 then return, but here
> logic got changed.
> Oh it seems ok, as platform calling i845 will have only one crtc.
Indeed. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
