On Tue, 26 Nov 2024, "Kahola, Mika" <mika.kahola@xxxxxxxxx> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Sent: Tuesday, 26 November 2024 11.30
>> To: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Sousa, Gustavo <gustavo.sousa@xxxxxxxxx>; Jadav, Raag
>> <raag.jadav@xxxxxxxxx>; Kahola, Mika <mika.kahola@xxxxxxxxx>
>> Subject: Re: [PATCH v5 1/2] drm/i915/xe3lpd: Power request
>> asserting/deasserting
>>
>> On Tue, 05 Nov 2024, Mika Kahola <mika.kahola@xxxxxxxxx> 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
