[haiku-commits] Re: r33749 - in haiku/trunk: headers/private/graphics/intel_extreme headers/private/shared src/add-ons/accelerants/intel_extreme src/add-ons/kernel/busses/agp_gart src/add-ons/kernel/drivers/graphics/intel_extreme
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Mon, 26 Oct 2009 11:51:34 +0100
Brecht Machiels <brecht@xxxxxxxxxxx> wrote:
> Axel Dörfler wrote:
> > brecht@xxxxxxxxxxx wrote:
> >> Added:
> >> haiku/trunk/headers/private/shared/binary-utils.h
> > We usually use the underscore instead of the dash to separate words
> > in
> > header names.
> I wasn't sure which to use, as in the same directory there is
> pci-utils.h, syscall_utils.h and usb-utils.h. So I decided it
> wouldn't
> matter.
Oh, I didn't know about those. In that case, I'd agree with you :-)
> > On a more specific note, you introduce a replacement for the STOLEN
> > _
> > MEMORY_MASK constant for the G4x series without proper attribution.
> > Since the extra bits are reserved in the earlier chipsets, this
> > just
> > doesn't make any sense. If it would make sense, it should be named
> > accordingly, and the original name should be adapted, too.
> As far as I can see, reserved bits could have any value, and thus the
> mask should not be enlarged for the older chipsets. The Intel
> datasheet
> seems to second that:
> """
> In many register, instruction and memory layout descriptions, certain
> bits are marked as “Reserved”. When bits are marked as reserved, it
> is
> essential for compatibility with future devices that software treat
> these bits as having a future, though unknown, effect. The behavior
> of
> reserved bits should be regarded as not only undefined, but
> unpredictable. Software should follow these guidelines in dealing
> with
> reserved bits:
> * Do not depend on the states of any reserved bits when testing
> values of registers that contain such bits. Mask out the reserved
> bits
> before testing. Do not depend on the states of any reserved bits when
> storing to instruction or to a register.
> """
I would insinuate that you are misunderstanding this paragraph: of
course you must not use reserved registers, or reserved bits in
registers. However, they are reserved for a reason: future use. And
since G4x, they now actually have a meaning - it's that exact future
use as mentioned in the paragraph. G4x is the successor of the earlier
chipsets, and those bits are no longer reserved.
> I do agree with using descriptive names for the constants however.
> I'll
> change the names accordingly and put the names used in the Intel
> datasheet in comments for future reference.
Okay, great.
> > Furthermore, I am not fond of the BINARY/BITMASK/BIT macros at all.
> > While I can grok the BINARY one, the BITMASK makes parsing actually
> > worse for me. And the BIT macro doesn't really help you either.
> > Defining masks as a hex number gives a lot more visual clue as to
> > what
> > it does than BITMASK.
> The BITMASK macro was defined to make it easier to convert how
> registers
> are described in the Intel documentation (and HW datasheets in
> general,
> I assume). I'm not sure what you mean with making parsing worse.
My own parsing of the code. I have no problems with visualizing the
effect of a 0x1fff mask, but I don't understand BITMASK() in the same
way.
[...]
> >> - if ((info.type & INTEL_TYPE_FAMILY_MASK) == INTEL_TYPE_9xx)
> >> + if ((info.type & INTEL_TYPE_GROUP_MASK) == INTEL_TYPE_G4x)
> >> + info.gtt_physical_base = info.display.u.h0.base_
> >> registers[mmioIndex]
> >> + + (2UL << 20);
> >> + else if ((info.type & INTEL_TYPE_FAMILY_MASK) == INTEL_TYPE_
> >> 9xx)
> >> info.gtt_physical_base = get_pci_config(info.display,
> > > i915_
> >> GTT_BASE, 4);
> > If that is actually the case, there is no reason for special casing
> > either.
> I'm not following you...
Not very surprising as I obviously left out half of the sentence :-)
I meant: if i915_GTT_BASE works also for the G4x (which it does IIRC),
then there is no need to special case it, and use the register base.
IIRC it's done in the same way for the i965, btw.
> > I hope you take this as constructive criticism - and in any case,
> > thanks
> > a lot for having figured this out! :-)
> Sure. But do bear with me. I have a long way to go :)
Well, just bear with me as well when I come across a little too strong.
Bye,
Axel.
Other related posts:
