On Wed, 07 Oct 2015, Ben Widawsky <ben@xxxxxxxxxxxx> wrote:
> On Tue, Oct 06, 2015 at 08:51:13PM +0000, Rodrigo Vivi wrote:
>> cc'ing Ben to get his opinion...
>>
>
> Of course anything is possible wrt the delta of KBL features vs SKL. With the
> knowledge we have, we can make a pretty educated guess that there will be no
> changes, and with an equally high level of confidence say that if there are
> changes, they will be very minor and self contained.
>
> I am in favor of this minimalistic patch myself. I think both the result, and
> reduced amount of churn make this patch favorable to the requested alternative.
> Some finer comments below.
>
>> On Tue, Oct 6, 2015 at 10:43 AM Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
>> wrote:
>>
>> > On Tue, 2015-10-06 at 12:24 +0300, Jani Nikula wrote:
>> > > On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
>> > > > Kabylake is gen 9.5 derivated from Skylake H0 stepping.
>
> Don't call it 9.5... some people don't like that... Just say it's a SKL
> derivative.
>
>> > > >
>> > > > So we don't need pre-production Skylake workaround and also
>> > > > firmware loading will use SKL H0 offsets.
>
> In fact we know some of these workarounds to be harmful.
>
>> > > >
>> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> > > > ---
>> > > > drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
>> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > > > b/drivers/gpu/drm/i915/i915_drv.h
>> > > > index 7374a0d..580c005 100644
>> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > > > @@ -2436,7 +2436,6 @@ struct drm_i915_cmd_table {
>> > > > })
>> > > > #define INTEL_INFO(p) (&__I915__(p)->info)
>> > > > #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id)
>> > > > -#define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision)
>> > > >
>> > > > #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577)
>> > > > #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562)
>> > > > @@ -2508,6 +2507,9 @@ struct drm_i915_cmd_table {
>> > > >
>> > > > #define IS_PRELIMINARY_HW(intel_info) ((intel_info)
>> > > > ->is_preliminary)
>> > > >
>> > > > +#define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision +
>> > > > \
>> > > > + IS_KABYLAKE(p) ? 7 : 0)
>> > > > +
>> > >
>> > > I am not fond of this at all. It will be really confusing that
>> > > ->revision is different from INTEL_REVID when checking the
>> > > workarounds,
>> > > and that you'll be using SKL_REVID_* to match KBL revision
>> > > ids.
>> >
>> > this is exactly one of the reasons why I did this sum in this way so
>> > they never match...
>> >
>
> Jani, I do understand your distaste with this patch. However, I think
> this is a very reasonable, and more importantly, readable wart in the
> code. It's very obvious what and why the macro does what it does.
INTEL_REVID() is supposed to be a shorthand for getting at
->dev->pdev->revision. I think it's rather surprising that on one
platform this would not hold. It's a clever trick, I'll give you that.
>> > > Additionally, we'll probably want to start removing SKL workarounds
>> > > before KBL workarounds.
>> >
>
> I don't see a reason for this. Maybe you have thought of something I haven't?
> We're not using any of the early SKL workarounds on KBL as a result of this
> patch, so it shouldn't matter.
>
>> > I believe this is another discussion... On HSW BDW I remember I was
>> > removing old Wa as it was no longer needed, but on SKL I saw this REVID
>> > and I believed the idea was to let them there since some devs might be
>> > using preliminary platforms yet for other reasons... I don't see a
>> > problem of letting the old W/a there.
>> >
>
> I'm very much in favor of killing pre-production support (not that anyone
> asked).
It was specifically dropping pre-production SKL workarounds I meant
above.
You might be interested in having a look at [1] I wrote to address the
revid/workaround part of the SKL vs. KBL discussion. With that, I think
we could keep INTEL_REVID() as it is, make IS_SKL_REVID() match only
SKL, and add IS_KBL_REVID() to match only KBL. As you say, most early
SKL workarounds don't impact KBL, so we wouldn't have to touch so many
places in the code to add KBL workarounds.
I also think that patch, incl. adding separate IS_KBL_REVID, is
orthogonal to whether IS_SKYLAKE() should match KBL or not.
[1] http://mid.gmane.org/1444131676-12187-3-git-send-email-jani.nikula@xxxxxxxxx
>> > >
>> > > Others may disagree, but I'd like KBL revid checks be different from
>> > > SKL.
>> > >
>> > > > #define SKL_REVID_A0 (0x0)
>> > > > #define SKL_REVID_B0 (0x1)
>> > > > #define SKL_REVID_C0 (0x2)
>> > > > @@ -2515,6 +2517,9 @@ struct drm_i915_cmd_table {
>> > > > #define SKL_REVID_E0 (0x4)
>> > > > #define SKL_REVID_F0 (0x5)
>> > > >
>> > > > +/* KBL A0 is based on SKL H0 */
>> > > > +#define KBL_REVID_A0 (0x7)
>> > >
>> > > You can't compare this against INTEL_REVID() now can you...? Or is
>> > > this
>> > > not the one in the spec? Confused already.
>> >
>> > Yes, this is confusing indeed. It seems that we have many levels of
>> > steppings (according to platform guys) and this platform stepping
>> > returning 0 is our KBL A0, but this correspond to our internal gpu
>> > stepping H0 (same going to skl h0).
>> >
>> > Like dmc firmware loading for instance we need to load the firmware for
>> > stepping 7.
>> >
>> > So yes, this definition matches BSPec KBL A0.
>> >
>
> Maybe amend the comment to say that the actual PCI header has revid 0 (or
> whatever it was). With that, it's pretty clear - and yes, it is a value which
> can and should be used to compare with INTEL_REVID, but as stated above, the
> comparison isn't needed, and if/when it is, we can/should revisit the more
> intrusive change you're suggesting.
I am still confused. I fear you may start thinking that no longer
supports my side of the argument. :(
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
