From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CA2A2C433EF for ; Fri, 4 Feb 2022 20:52:08 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 1537310E478; Fri, 4 Feb 2022 20:52:08 +0000 (UTC) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id C0A8110E478 for ; Fri, 4 Feb 2022 20:52:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1644007926; x=1675543926; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=PZ4WyPfM8pg4buV5yx/Y7Jyu1f0LQ1dosc8zMvmNhHA=; b=D4IKD+8LOtI/mJWmSb84rHvLTgeep2YXbXU/HedOhq5I6X8QAy5gdR/R kqKmH9yErcS/DrolL7lMkqg7+el+HlBQyPW/XtNNPYu421gRGV763sPPz PpG19hMrHxDRsJm8LBHHiv8/kHl9NlYFDaBSytUDjl2W/e4e25PRXVYBq DHb593y0ba/j7Lvd+JJ/XX4ru3coZSUULJMfXA20h59ej0mAyeiMvK+OF U3FeLqKhbZqEurhModHBhhbwUdaGPFf3ejhW1Y2vIfrCiOqSNopbRMgKt tUjS7ejkikICfa69HNckBNPk8+b0Ad0O5y4nMjPkl0stXLGbO5zACdpQF Q==; X-IronPort-AV: E=McAfee;i="6200,9189,10248"; a="334835999" X-IronPort-AV: E=Sophos;i="5.88,343,1635231600"; d="scan'208";a="334835999" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Feb 2022 12:52:05 -0800 X-IronPort-AV: E=Sophos;i="5.88,343,1635231600"; d="scan'208";a="699795112" Received: from labuser-z97x-ud5h.jf.intel.com (HELO labuser-Z97X-UD5H) ([10.165.21.211]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Feb 2022 12:52:05 -0800 Date: Fri, 4 Feb 2022 12:52:01 -0800 From: "Navare, Manasi" To: Ville Syrjala Message-ID: <20220204205156.GA22898@labuser-Z97X-UD5H> References: <20220203183823.22890-5-ville.syrjala@linux.intel.com> <20220204072049.1610-1-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220204072049.1610-1-ville.syrjala@linux.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [Intel-gfx] [PATCH v2 04/10] drm/i915: Clean up the bigjoiner state copy logic X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Fri, Feb 04, 2022 at 09:20:49AM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > Currently the bigjoiner state copy logic is kind of > a byzantine mess. > > Clean it up to operate in the following manner during a full > modeset: > 1) master uapi -> hw state copy > 2) master hw -> slave hw state copy > > And during a non-modeset update we do: > 1) master uapi -> hw state light copy > 2) master hw -> slave hw state light copy > > I think that is now easier to reason about since we never do > any kind of master uapi -> slave hw state copy short circuit > that could happen previously. > > Obviously this does now depend on the master uapi->hw copy > always happening before the master hw -> slave hw copy, but > that is guaranteed by the fact that we always add both crtcs > to the state early, the crtcs are registered in pipe > order (so the compute_config loop happens in pipe order), > and the hardware requires the master pipe has to be lower > than the slave pipe as well. And for good measure we shall > add a check+WARN for this before doing the bigjoiner crtc > assignment. > > v2: Fix uapi.ctm vs. hw.ctm copy-paste fail > > Signed-off-by: Ville Syrjälä Looks good to me, jus one question on how we decide what state from orginal slave crtc state to preserve. But in either case Reviewed-by: Manasi Navare > --- > drivers/gpu/drm/i915/display/intel_atomic.c | 11 -- > drivers/gpu/drm/i915/display/intel_atomic.h | 2 - > drivers/gpu/drm/i915/display/intel_display.c | 170 ++++++++++++------- > 3 files changed, 108 insertions(+), 75 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c > index 093904065112..e0667d163266 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > @@ -281,17 +281,6 @@ void intel_crtc_free_hw_state(struct intel_crtc_state *crtc_state) > intel_crtc_put_color_blobs(crtc_state); > } > > -void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state, > - const struct intel_crtc_state *from_crtc_state) > -{ > - drm_property_replace_blob(&crtc_state->hw.degamma_lut, > - from_crtc_state->uapi.degamma_lut); > - drm_property_replace_blob(&crtc_state->hw.gamma_lut, > - from_crtc_state->uapi.gamma_lut); > - drm_property_replace_blob(&crtc_state->hw.ctm, > - from_crtc_state->uapi.ctm); > -} > - > /** > * intel_crtc_destroy_state - destroy crtc state > * @crtc: drm crtc > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h > index d2700c74c9da..1dc439983dd9 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.h > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h > @@ -44,8 +44,6 @@ struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc); > void intel_crtc_destroy_state(struct drm_crtc *crtc, > struct drm_crtc_state *state); > void intel_crtc_free_hw_state(struct intel_crtc_state *crtc_state); > -void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state, > - const struct intel_crtc_state *from_crtc_state); > struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev); > void intel_atomic_state_free(struct drm_atomic_state *state); > void intel_atomic_state_clear(struct drm_atomic_state *state); > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 349cc3807e8b..b391cb98b12f 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -5818,32 +5818,37 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state) > > static void > intel_crtc_copy_uapi_to_hw_state_nomodeset(struct intel_atomic_state *state, > - struct intel_crtc_state *crtc_state) > + struct intel_crtc *crtc) > { > - const struct intel_crtc_state *master_crtc_state; > - struct intel_crtc *master_crtc; > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > > - master_crtc = intel_master_crtc(crtc_state); > - master_crtc_state = intel_atomic_get_new_crtc_state(state, master_crtc); > + WARN_ON(crtc_state->bigjoiner_slave); > > - /* No need to copy state if the master state is unchanged */ > - if (master_crtc_state) { > - crtc_state->uapi.color_mgmt_changed = master_crtc_state->uapi.color_mgmt_changed; > - intel_crtc_copy_color_blobs(crtc_state, master_crtc_state); > - } > + drm_property_replace_blob(&crtc_state->hw.degamma_lut, > + crtc_state->uapi.degamma_lut); > + drm_property_replace_blob(&crtc_state->hw.gamma_lut, > + crtc_state->uapi.gamma_lut); > + drm_property_replace_blob(&crtc_state->hw.ctm, > + crtc_state->uapi.ctm); > } > > static void > -intel_crtc_copy_uapi_to_hw_state(struct intel_atomic_state *state, > - struct intel_crtc_state *crtc_state) > +intel_crtc_copy_uapi_to_hw_state_modeset(struct intel_atomic_state *state, > + struct intel_crtc *crtc) > { > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > + > + WARN_ON(crtc_state->bigjoiner_slave); > + > crtc_state->hw.enable = crtc_state->uapi.enable; > crtc_state->hw.active = crtc_state->uapi.active; > crtc_state->hw.mode = crtc_state->uapi.mode; > crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode; > crtc_state->hw.scaling_filter = crtc_state->uapi.scaling_filter; > > - intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc_state); > + intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc); > } > > static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state) > @@ -5859,7 +5864,6 @@ static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state > crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode; > crtc_state->uapi.scaling_filter = crtc_state->hw.scaling_filter; > > - /* copy color blobs to uapi */ > drm_property_replace_blob(&crtc_state->uapi.degamma_lut, > crtc_state->hw.degamma_lut); > drm_property_replace_blob(&crtc_state->uapi.gamma_lut, > @@ -5868,61 +5872,79 @@ static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state > crtc_state->hw.ctm); > } > > +static void > +copy_bigjoiner_crtc_state_nomodeset(struct intel_atomic_state *state, > + struct intel_crtc *slave_crtc) > +{ > + struct intel_crtc_state *slave_crtc_state = > + intel_atomic_get_new_crtc_state(state, slave_crtc); > + struct intel_crtc *master_crtc = intel_master_crtc(slave_crtc_state); > + const struct intel_crtc_state *master_crtc_state = > + intel_atomic_get_new_crtc_state(state, master_crtc); > + > + drm_property_replace_blob(&slave_crtc_state->hw.degamma_lut, > + master_crtc_state->hw.degamma_lut); > + drm_property_replace_blob(&slave_crtc_state->hw.gamma_lut, > + master_crtc_state->hw.gamma_lut); > + drm_property_replace_blob(&slave_crtc_state->hw.ctm, > + master_crtc_state->hw.ctm); > + > + slave_crtc_state->uapi.color_mgmt_changed = master_crtc_state->uapi.color_mgmt_changed; > +} > + > static int > -copy_bigjoiner_crtc_state(struct intel_crtc_state *crtc_state, > - const struct intel_crtc_state *from_crtc_state) > +copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *state, > + struct intel_crtc *slave_crtc) > { > + struct intel_crtc_state *slave_crtc_state = > + intel_atomic_get_new_crtc_state(state, slave_crtc); > + struct intel_crtc *master_crtc = intel_master_crtc(slave_crtc_state); > + const struct intel_crtc_state *master_crtc_state = > + intel_atomic_get_new_crtc_state(state, master_crtc); > struct intel_crtc_state *saved_state; > > - saved_state = kmemdup(from_crtc_state, sizeof(*saved_state), GFP_KERNEL); > + saved_state = kmemdup(master_crtc_state, sizeof(*saved_state), GFP_KERNEL); > if (!saved_state) > return -ENOMEM; > > - saved_state->uapi = crtc_state->uapi; > - saved_state->scaler_state = crtc_state->scaler_state; > - saved_state->shared_dpll = crtc_state->shared_dpll; > - saved_state->dpll_hw_state = crtc_state->dpll_hw_state; > - saved_state->crc_enabled = crtc_state->crc_enabled; > + /* preserve some things from the slave's original crtc state */ > + saved_state->uapi = slave_crtc_state->uapi; > + saved_state->scaler_state = slave_crtc_state->scaler_state; > + saved_state->shared_dpll = slave_crtc_state->shared_dpll; > + saved_state->dpll_hw_state = slave_crtc_state->dpll_hw_state; > + saved_state->crc_enabled = slave_crtc_state->crc_enabled; Slave crtc state here not set at all , so why do we preserve the things from slave's original crtc state and how we decide on what all to preserve? Manasi > > - intel_crtc_free_hw_state(crtc_state); > - memcpy(crtc_state, saved_state, sizeof(*crtc_state)); > + intel_crtc_free_hw_state(slave_crtc_state); > + memcpy(slave_crtc_state, saved_state, sizeof(*slave_crtc_state)); > kfree(saved_state); > > /* Re-init hw state */ > - memset(&crtc_state->hw, 0, sizeof(saved_state->hw)); > - crtc_state->hw.enable = from_crtc_state->hw.enable; > - crtc_state->hw.active = from_crtc_state->hw.active; > - crtc_state->hw.mode = from_crtc_state->hw.mode; > - crtc_state->hw.pipe_mode = from_crtc_state->hw.pipe_mode; > - crtc_state->hw.adjusted_mode = from_crtc_state->hw.adjusted_mode; > - crtc_state->hw.scaling_filter = from_crtc_state->hw.scaling_filter; > + memset(&slave_crtc_state->hw, 0, sizeof(slave_crtc_state->hw)); > + slave_crtc_state->hw.enable = master_crtc_state->hw.enable; > + slave_crtc_state->hw.active = master_crtc_state->hw.active; > + slave_crtc_state->hw.mode = master_crtc_state->hw.mode; > + slave_crtc_state->hw.pipe_mode = master_crtc_state->hw.pipe_mode; > + slave_crtc_state->hw.adjusted_mode = master_crtc_state->hw.adjusted_mode; > + slave_crtc_state->hw.scaling_filter = master_crtc_state->hw.scaling_filter; > > - drm_property_replace_blob(&crtc_state->hw.degamma_lut, > - from_crtc_state->hw.degamma_lut); > - drm_property_replace_blob(&crtc_state->hw.gamma_lut, > - from_crtc_state->hw.gamma_lut); > - drm_property_replace_blob(&crtc_state->hw.ctm, > - from_crtc_state->hw.ctm); > + copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc); > > /* Some fixups */ > - crtc_state->uapi.mode_changed = from_crtc_state->uapi.mode_changed; > - crtc_state->uapi.connectors_changed = from_crtc_state->uapi.connectors_changed; > - crtc_state->uapi.active_changed = from_crtc_state->uapi.active_changed; > - crtc_state->uapi.color_mgmt_changed = from_crtc_state->uapi.color_mgmt_changed; > - crtc_state->nv12_planes = crtc_state->c8_planes = crtc_state->update_planes = 0; > - crtc_state->bigjoiner_linked_crtc = to_intel_crtc(from_crtc_state->uapi.crtc); > - crtc_state->bigjoiner_slave = true; > - crtc_state->cpu_transcoder = from_crtc_state->cpu_transcoder; > - crtc_state->has_audio = from_crtc_state->has_audio; > + slave_crtc_state->uapi.mode_changed = master_crtc_state->uapi.mode_changed; > + slave_crtc_state->uapi.connectors_changed = master_crtc_state->uapi.connectors_changed; > + slave_crtc_state->uapi.active_changed = master_crtc_state->uapi.active_changed; > + slave_crtc_state->cpu_transcoder = master_crtc_state->cpu_transcoder; > + slave_crtc_state->has_audio = master_crtc_state->has_audio; > > return 0; > } > > static int > intel_crtc_prepare_cleared_state(struct intel_atomic_state *state, > - struct intel_crtc_state *crtc_state) > + struct intel_crtc *crtc) > { > - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > + struct intel_crtc_state *crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > struct intel_crtc_state *saved_state; > > @@ -5952,7 +5974,7 @@ intel_crtc_prepare_cleared_state(struct intel_atomic_state *state, > memcpy(crtc_state, saved_state, sizeof(*crtc_state)); > kfree(saved_state); > > - intel_crtc_copy_uapi_to_hw_state(state, crtc_state); > + intel_crtc_copy_uapi_to_hw_state_modeset(state, crtc); > > return 0; > } > @@ -7592,6 +7614,9 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > struct intel_crtc_state *slave_crtc_state; > struct intel_crtc *slave_crtc; > > + WARN_ON(master_crtc_state->bigjoiner_linked_crtc); > + WARN_ON(master_crtc_state->bigjoiner_slave); > + > if (!master_crtc_state->bigjoiner) > return 0; > > @@ -7604,7 +7629,6 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > return -EINVAL; > } > > - master_crtc_state->bigjoiner_linked_crtc = slave_crtc; > slave_crtc_state = intel_atomic_get_crtc_state(&state->base, slave_crtc); > if (IS_ERR(slave_crtc_state)) > return PTR_ERR(slave_crtc_state); > @@ -7613,11 +7637,28 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > if (slave_crtc_state->uapi.enable) > goto claimed; > > + /* > + * The state copy logic assumes the master crtc gets processed > + * before the slave crtc during the main compute_config loop. > + * This works because the crtcs are created in pipe order, > + * and the hardware requires master pipe < slave pipe as well. > + * Should that change we need to rethink the logic. > + */ > + if (WARN_ON(drm_crtc_index(&master_crtc->base) > drm_crtc_index(&slave_crtc->base))) > + return -EINVAL; > + > drm_dbg_kms(&i915->drm, > - "[CRTC:%d:%s] Used as slave for big joiner\n", > - slave_crtc->base.base.id, slave_crtc->base.name); > + "[CRTC:%d:%s] Used as slave for big joiner master [CRTC:%d:%s]\n", > + slave_crtc->base.base.id, slave_crtc->base.name, > + master_crtc->base.base.id, master_crtc->base.name); > > - return copy_bigjoiner_crtc_state(slave_crtc_state, master_crtc_state); > + master_crtc_state->bigjoiner_linked_crtc = slave_crtc; > + master_crtc_state->bigjoiner_slave = false; > + > + slave_crtc_state->bigjoiner_linked_crtc = master_crtc; > + slave_crtc_state->bigjoiner_slave = true; > + > + return copy_bigjoiner_crtc_state_modeset(state, slave_crtc); > > claimed: > drm_dbg_kms(&i915->drm, > @@ -7629,15 +7670,19 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > } > > static void kill_bigjoiner_slave(struct intel_atomic_state *state, > - struct intel_crtc_state *master_crtc_state) > + struct intel_crtc *master_crtc) > { > + struct intel_crtc_state *master_crtc_state = > + intel_atomic_get_new_crtc_state(state, master_crtc); > + struct intel_crtc *slave_crtc = master_crtc_state->bigjoiner_linked_crtc; > struct intel_crtc_state *slave_crtc_state = > - intel_atomic_get_new_crtc_state(state, master_crtc_state->bigjoiner_linked_crtc); > + intel_atomic_get_new_crtc_state(state, slave_crtc); > > slave_crtc_state->bigjoiner = master_crtc_state->bigjoiner = false; > slave_crtc_state->bigjoiner_slave = master_crtc_state->bigjoiner_slave = false; > slave_crtc_state->bigjoiner_linked_crtc = master_crtc_state->bigjoiner_linked_crtc = NULL; > - intel_crtc_copy_uapi_to_hw_state(state, slave_crtc_state); > + > + intel_crtc_copy_uapi_to_hw_state_modeset(state, slave_crtc); > } > > /** > @@ -7823,7 +7868,7 @@ static int intel_bigjoiner_add_affected_crtcs(struct intel_atomic_state *state) > /* Kill old bigjoiner link, we may re-establish afterwards */ > if (intel_crtc_needs_modeset(crtc_state) && > crtc_state->bigjoiner && !crtc_state->bigjoiner_slave) > - kill_bigjoiner_slave(state, crtc_state); > + kill_bigjoiner_slave(state, crtc); > } > > return 0; > @@ -7867,21 +7912,22 @@ static int intel_atomic_check(struct drm_device *dev, > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > new_crtc_state, i) { > if (!intel_crtc_needs_modeset(new_crtc_state)) { > - /* Light copy */ > - intel_crtc_copy_uapi_to_hw_state_nomodeset(state, new_crtc_state); > - > + if (new_crtc_state->bigjoiner_slave) > + copy_bigjoiner_crtc_state_nomodeset(state, crtc); > + else > + intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc); > continue; > } > > if (!new_crtc_state->uapi.enable) { > if (!new_crtc_state->bigjoiner_slave) { > - intel_crtc_copy_uapi_to_hw_state(state, new_crtc_state); > + intel_crtc_copy_uapi_to_hw_state_modeset(state, crtc); > any_ms = true; > } > continue; > } > > - ret = intel_crtc_prepare_cleared_state(state, new_crtc_state); > + ret = intel_crtc_prepare_cleared_state(state, crtc); > if (ret) > goto fail; > > -- > 2.34.1 >