> -----Original Message----- > From: Jadav, Raag <raag.jadav@xxxxxxxxx> > Sent: Monday, 28 October 2024 16.43 > To: Sousa, Gustavo <gustavo.sousa@xxxxxxxxx> > Cc: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] drm/i915/xe3lpd: Power request asserting/deasserting > > On Mon, Oct 28, 2024 at 11:21:59AM -0300, Gustavo Sousa wrote: > > Quoting Raag Jadav (2024-10-28 11:02:02-03:00) > > >On Mon, Oct 28, 2024 at 02:58:35PM +0200, Mika Kahola wrote: > > > > > >... > > > > > >> +static bool wa_tcss_power_request_assert(struct drm_i915_private *i915, > > >> + bool enable) { > > >> + /* > > >> + * Limit to PTL only > > >> + * TODO: Add check for PICA IP and use that instead limiting WA for > > >> + * platform > > >> + */ > > >> + if (DISPLAY_VER(i915) != 30) > > > > > >Not sure if hardcoding constants is the best of the approach, but I > > >found similar patterns in other places, so upto you. > > > > Using version number directly makes it easier to concurrently merge > > independent patches for some display IP without having to wait some > > #define to become available. That comes with the cost of having to > > remember the version numbers (or checking somewhere) though. > > Agree. Although usually we have codenames for this, which is easier to track or > grep. > > > > > > >> + return true; > > >> + > > >> + /* check if mailbox is running busy */ > > >> + if (intel_de_wait_for_clear(i915, TCSS_DISP_MAILBOX_IN_CMD, > > >> + TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY, 10)) { > > >> + drm_dbg_kms(&i915->drm, > > >> + "Power request assert WA timeout > > >> + waiting for TCSS mailbox run/busy bit to clear\n"); > > > > > >Is timeout considered a failure? Not sure _dbg is the right helper if it. > > > > I believe it is okay to have _dbg here, because the returned value is > > checked and a warning is raised. > > Which makes the logging redundant... > > > Althought we could make this more > > self-contained by jumping an error path inside this function. > > ... but if we'd still want it I think this will be best. > > > > > > > > >> + return false; > > >> + } > > >> + > > >> + if (enable) > > >> + intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, 1); > > >> + else > > >> + intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, > > >> + 0); > > > > > >Since enable it already a bool, this can be > > > > > > intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, enable); > > > > > >> + > > >> + intel_de_write(i915, 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 */ > > > > > >Any specific reason to do this on exit? > > >I'm assuming anyone trying to use the mailbox further down would be > > >doing this anyway since it's a prerequisite, right? > > > > This wait is part of the "DE to IOM Mailbox" flow. I believe this is > > necessary and the workaround might even not be effective without it. > > My understanding is that the wait is for the availability of mailbox which is not > equivalent to command success, but I could be wrong. I think these two are related. We could consider command to be successful if mailbox is free to use within a certain timeframe. Otherwise, I would consider that there might be something wrong with the communication or command hasn't been successfully executed. -Mika- > > Raag