RE: [PATCH v5 1/2] drm/i915/xe3lpd: Power request asserting/deasserting

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 26 Nov 2024, "Kahola, Mika" <mika.kahola@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@linux.intel.com>
>> Sent: Tuesday, 26 November 2024 11.30
>> To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org
>> Cc: Sousa, Gustavo <gustavo.sousa@intel.com>; Jadav, Raag
>> <raag.jadav@intel.com>; Kahola, Mika <mika.kahola@intel.com>
>> Subject: Re: [PATCH v5 1/2] drm/i915/xe3lpd: Power request
>> asserting/deasserting
>> 
>> On Tue, 05 Nov 2024, Mika Kahola <mika.kahola@intel.com> wrote:
>> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
>> > b/drivers/gpu/drm/i915/display/intel_tc.c
>> > index b16c4d2d4077..e40d55f4c0c4 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
>> > @@ -1013,6 +1013,30 @@ xelpdp_tc_phy_wait_for_tcss_power(struct
>> intel_tc_port *tc, bool enabled)
>> >  	return true;
>> >  }
>> >
>> > +static void wa_14020908590(struct intel_display *display, bool
>> > +enable)
>> 
>> Yeah I still don't like functions named wa_14020908590. It's meaningless. What
>> does it do?
> That's a good point. We do have few functions in our driver that have workaround number in its name.
>
> What would be the better way? Add a comment that references to workaround number and have a meaningful function name?

Yes. We have a somewhat standardized format for that.

>
>> 
>> > +{
>> > +	/* check if mailbox is running busy */
>> > +	if (intel_de_wait_for_clear(display, TCSS_DISP_MAILBOX_IN_CMD,
>> > +				    TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY,
>> 10)) {
>> > +		drm_dbg_kms(display->drm,
>> > +			    "Timeout waiting for TCSS mailbox run/busy bit to
>> clear\n");
>> > +		return;
>> > +	}
>> > +
>> > +	intel_de_write(display, TCSS_DISP_MAILBOX_IN_DATA, enable);
>> 
>> Not a fan of bool -> u32 implicit conversion here, with the register contents not
>> described.
> Ok. I will modify this to use u32 instead.

Of course, the parameter may still be bool, but would be better to be
more explicit about what's written to the register.

>
>> 
>> > +	intel_de_write(display, TCSS_DISP_MAILBOX_IN_CMD,
>> > +		       TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY |
>> > +		       TCSS_DISP_MAILBOX_IN_CMD_DATA(0x1));
>> > +
>> > +	/* wait to clear mailbox running busy bit before continuing */
>> > +	if (intel_de_wait_for_clear(display, TCSS_DISP_MAILBOX_IN_CMD,
>> > +				    TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY,
>> 10)) {
>> > +		drm_dbg_kms(display->drm,
>> > +			    "Timeout after writing data to mailbox. Mailbox
>> run/busy bit did not clear\n");
>> > +		return;
>> > +	}
>> > +}
>> > +
>> >  static void __xelpdp_tc_phy_enable_tcss_power(struct intel_tc_port
>> > *tc, bool enable)  {
>> >  	struct drm_i915_private *i915 = tc_to_i915(tc); @@ -1022,6 +1046,13
>> > @@ static void __xelpdp_tc_phy_enable_tcss_power(struct intel_tc_port
>> > *tc, bool ena
>> >
>> >  	assert_tc_cold_blocked(tc);
>> >
>> > +	/*
>> > +	 * Gfx driver WA 14020908590 for PTL tcss_rxdetect_clkswb_req/ack
>> > +	 * handshake violation when pwwreq= 0->1 during TC7/10 entry
>> > +	 */
>> > +	if (DISPLAY_VER(i915) == 30)
>> > +		wa_14020908590(&i915->display, enable);
>> 
>> You should add
>> 
>> 	struct intel_display *display = &i915->display;
>> 
>> local variable already in this patch, so the next patch doesn't have to modify the
>> above line again. You can do the subsequent conversions in the follow-up.
> Ok. I will make this change
>
> Thanks for the review!
>
> -Mika-
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> > +
>> >  	val = intel_de_read(i915, reg);
>> >  	if (enable)
>> >  		val |= XELPDP_TCSS_POWER_REQUEST;
>> 
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel