Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)

2012-05-18 Thread Alan Coopersmith 
On 05/15/12 11:52 AM, Dave Airlie wrote:
 Pass the ScrnInfoPtr instead of the index in the int10 struct.
 
 This saves us using it to dereference xf86Screens.
 
 v2: address Alan's comment to fix struct alignment.
 
 Signed-off-by: Dave Airlie airl...@redhat.com

Reviewed-by: Alan Coopersmith alan.coopersm...@oracle.com


-- 
-Alan Coopersmith-  alan.coopersm...@oracle.com
 Oracle Solaris Engineering - http://blogs.oracle.com/alanc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)

2012-05-17 Thread Adam Jackson 

On 5/16/12 5:16 AM, Dave Airlie wrote:



@@ -340,7 +341,7 @@ vbeDoEDID(vbeInfoPtr pVbe, pointer pDDCModule)
  if (!DDC_data)
  return NULL;

-pMonitor = xf86InterpretEDID(pVbe-pInt10-scrnIndex, DDC_data);
+pMonitor = xf86InterpretEDID(pVbe-pInt10-pScrn-scrnIndex,
DDC_data);



The callee here wants index-ptr conversion too, doesn't it?  I don't think
I see that in subsequent patches.


That gets into an area I've been thinking about but mostly avoiding for now,

That is pretty much a logging function, i.e. scrnIndex only goes into
logging functions,

Now I'm tempted to leave logging functions just passing indices, but
I'm thinking its probably a bad idea long term, just not sure what it
is short-term.


Point taken, hadn't thought about that.  You do actually want _some_ 
integer there, otherwise how do you distinguish two nouveau instances.


I suppose as long as the screen arrays become things that it's 
physically not possible to index into then it really doesn't matter what 
scrnIndex is numerically, and it'd be safe to keep using as a unique ID.


R-b for the original patch.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)

2012-05-16 Thread Dave Airlie 

 @@ -340,7 +341,7 @@ vbeDoEDID(vbeInfoPtr pVbe, pointer pDDCModule)
      if (!DDC_data)
          return NULL;

 -    pMonitor = xf86InterpretEDID(pVbe-pInt10-scrnIndex, DDC_data);
 +    pMonitor = xf86InterpretEDID(pVbe-pInt10-pScrn-scrnIndex,
 DDC_data);


 The callee here wants index-ptr conversion too, doesn't it?  I don't think
 I see that in subsequent patches.

That gets into an area I've been thinking about but mostly avoiding for now,

That is pretty much a logging function, i.e. scrnIndex only goes into
logging functions,

Now I'm tempted to leave logging functions just passing indices, but
I'm thinking its probably a bad idea long term, just not sure what it
is short-term.

I was thinking of just making scrnIndex for GPU screens have a
higher-bit set in them, and the logging would understand that and
strip it out, or maybe creating a new logging index that gets passed
to all the logging functions.

However I was playing with using a combo or scrnIndex and isGPU to
denote a GPU screen, and it seems cleaner, but the big problem is
changing the logging function signatures is a major amount of work,
the API changes I've been making are moderate in comparison, but
everyone calls the logging functions from some really wierd places so
there would be a lot of audit.

So I think thinking short-term, I just do the high-bit or start gpu
screens after MAXSCREENS, and make sure to never expose that
implementation detail to drivers, then once we are past the worst of
this we can contemplate some way to fix the logging interfaces.

Dave.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)

2012-05-16 Thread Mark Kettenis 
 Date: Wed, 16 May 2012 10:16:24 +0100
 From: Dave Airlie airl...@gmail.com
 
 
  @@ -340,7 +341,7 @@ vbeDoEDID(vbeInfoPtr pVbe, pointer pDDCModule)
       if (!DDC_data)
           return NULL;
 
  -    pMonitor = xf86InterpretEDID(pVbe-pInt10-scrnIndex, DDC_data);
  +    pMonitor = xf86InterpretEDID(pVbe-pInt10-pScrn-scrnIndex,
  DDC_data);
 
 
  The callee here wants index-ptr conversion too, doesn't it?  I don't think
  I see that in subsequent patches.
 
 That gets into an area I've been thinking about but mostly avoiding for now,
 
 That is pretty much a logging function, i.e. scrnIndex only goes into
 logging functions,
 
 Now I'm tempted to leave logging functions just passing indices, but
 I'm thinking its probably a bad idea long term, just not sure what it
 is short-term.
 
 I was thinking of just making scrnIndex for GPU screens have a
 higher-bit set in them, and the logging would understand that and
 strip it out, or maybe creating a new logging index that gets passed
 to all the logging functions.
 
 However I was playing with using a combo or scrnIndex and isGPU to
 denote a GPU screen, and it seems cleaner, but the big problem is
 changing the logging function signatures is a major amount of work,
 the API changes I've been making are moderate in comparison, but
 everyone calls the logging functions from some really wierd places so
 there would be a lot of audit.
 
 So I think thinking short-term, I just do the high-bit or start gpu
 screens after MAXSCREENS, and make sure to never expose that
 implementation detail to drivers, then once we are past the worst of
 this we can contemplate some way to fix the logging interfaces.

How about adding an interface that converts a screen pointer into a
string of some sorts and use that in the logging functions?
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)

2012-05-16 Thread Dave Airlie 
On Wed, May 16, 2012 at 10:24 AM, Mark Kettenis mark.kette...@xs4all.nl wrote:
 Date: Wed, 16 May 2012 10:16:24 +0100
 From: Dave Airlie airl...@gmail.com

 
  @@ -340,7 +341,7 @@ vbeDoEDID(vbeInfoPtr pVbe, pointer pDDCModule)
       if (!DDC_data)
           return NULL;
 
  -    pMonitor = xf86InterpretEDID(pVbe-pInt10-scrnIndex, DDC_data);
  +    pMonitor = xf86InterpretEDID(pVbe-pInt10-pScrn-scrnIndex,
  DDC_data);
 
 
  The callee here wants index-ptr conversion too, doesn't it?  I don't think
  I see that in subsequent patches.

 That gets into an area I've been thinking about but mostly avoiding for now,

 That is pretty much a logging function, i.e. scrnIndex only goes into
 logging functions,

 Now I'm tempted to leave logging functions just passing indices, but
 I'm thinking its probably a bad idea long term, just not sure what it
 is short-term.

 I was thinking of just making scrnIndex for GPU screens have a
 higher-bit set in them, and the logging would understand that and
 strip it out, or maybe creating a new logging index that gets passed
 to all the logging functions.

 However I was playing with using a combo or scrnIndex and isGPU to
 denote a GPU screen, and it seems cleaner, but the big problem is
 changing the logging function signatures is a major amount of work,
 the API changes I've been making are moderate in comparison, but
 everyone calls the logging functions from some really wierd places so
 there would be a lot of audit.

 So I think thinking short-term, I just do the high-bit or start gpu
 screens after MAXSCREENS, and make sure to never expose that
 implementation detail to drivers, then once we are past the worst of
 this we can contemplate some way to fix the logging interfaces.

 How about adding an interface that converts a screen pointer into a
 string of some sorts and use that in the logging functions?

Well we currently have one that turns the scrnIndex into a string
inside the logging functions
so just changing scrnIndex to ScrnInfoPtr would be fine with NULL
doing no string, but the sheer
number of callsites is overwhelming and keeping drivers building
against old/new servers would
also hurt, which are the main reasons I'd be punting on this for now.

Dave.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)

2012-05-16 Thread Mark Kettenis 
 Date: Wed, 16 May 2012 10:31:26 +0100
 From: Dave Airlie airl...@gmail.com
 
 On Wed, May 16, 2012 at 10:24 AM, Mark Kettenis mark.kette...@xs4all.nl 
 wrote:
  Date: Wed, 16 May 2012 10:16:24 +0100
  From: Dave Airlie airl...@gmail.com
 
  
   @@ -340,7 +341,7 @@ vbeDoEDID(vbeInfoPtr pVbe, pointer pDDCModule)
        if (!DDC_data)
            return NULL;
  
   -    pMonitor = xf86InterpretEDID(pVbe-pInt10-scrnIndex, DDC_data);
   +    pMonitor = xf86InterpretEDID(pVbe-pInt10-pScrn-scrnIndex,
   DDC_data);
  
  
   The callee here wants index-ptr conversion too, doesn't it?  I don't 
   think
   I see that in subsequent patches.
 
  That gets into an area I've been thinking about but mostly avoiding for 
  now,
 
  That is pretty much a logging function, i.e. scrnIndex only goes into
  logging functions,
 
  Now I'm tempted to leave logging functions just passing indices, but
  I'm thinking its probably a bad idea long term, just not sure what it
  is short-term.
 
  I was thinking of just making scrnIndex for GPU screens have a
  higher-bit set in them, and the logging would understand that and
  strip it out, or maybe creating a new logging index that gets passed
  to all the logging functions.
 
  However I was playing with using a combo or scrnIndex and isGPU to
  denote a GPU screen, and it seems cleaner, but the big problem is
  changing the logging function signatures is a major amount of work,
  the API changes I've been making are moderate in comparison, but
  everyone calls the logging functions from some really wierd places so
  there would be a lot of audit.
 
  So I think thinking short-term, I just do the high-bit or start gpu
  screens after MAXSCREENS, and make sure to never expose that
  implementation detail to drivers, then once we are past the worst of
  this we can contemplate some way to fix the logging interfaces.
 
  How about adding an interface that converts a screen pointer into a
  string of some sorts and use that in the logging functions?
 
 Well we currently have one that turns the scrnIndex into a string
 inside the logging functions
 so just changing scrnIndex to ScrnInfoPtr would be fine with NULL
 doing no string, but the sheer
 number of callsites is overwhelming and keeping drivers building
 against old/new servers would
 also hurt, which are the main reasons I'd be punting on this for now.

Fair enough.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)

2012-05-15 Thread Dave Airlie 
Pass the ScrnInfoPtr instead of the index in the int10 struct.

This saves us using it to dereference xf86Screens.

v2: address Alan's comment to fix struct alignment.

Signed-off-by: Dave Airlie airl...@redhat.com
---
 hw/xfree86/int10/generic.c |2 +-
 hw/xfree86/int10/helper_exec.c |   18 
 hw/xfree86/int10/helper_mem.c  |8 +++---
 hw/xfree86/int10/xf86int10.c   |   42 
 hw/xfree86/int10/xf86int10.h   |4 +-
 hw/xfree86/vbe/vbe.c   |   17 ---
 6 files changed, 46 insertions(+), 45 deletions(-)

diff --git a/hw/xfree86/int10/generic.c b/hw/xfree86/int10/generic.c
index 4b97520..5343e47 100644
--- a/hw/xfree86/int10/generic.c
+++ b/hw/xfree86/int10/generic.c
@@ -89,7 +89,7 @@ xf86ExtendedInitInt10(int entityIndex, int Flags)
 pInt-mem = genericMem;
 pInt-private = (pointer) xnfcalloc(1, sizeof(genericInt10Priv));
 INTPriv(pInt)-alloc = (pointer) xnfcalloc(1, 
ALLOC_ENTRIES(getpagesize()));
-pInt-scrnIndex = pScrn-scrnIndex;
+pInt-pScrn = pScrn;
 base = INTPriv(pInt)-base = xnfalloc(SYS_BIOS);
 
 /* FIXME: Shouldn't this be a failure case?  Leaving dev as NULL seems like
diff --git a/hw/xfree86/int10/helper_exec.c b/hw/xfree86/int10/helper_exec.c
index 1e90877..1c58cf7 100644
--- a/hw/xfree86/int10/helper_exec.c
+++ b/hw/xfree86/int10/helper_exec.c
@@ -125,7 +125,7 @@ run_bios_int(int num, xf86Int10InfoPtr pInt)
 if (MEM_RW(pInt, (num  2) + 2) == (SYS_BIOS  4)) {  /* 
SYS_BIOS_SEG ? */
 
 if (num == 21  X86_AH == 0x4e) {
-xf86DrvMsg(pInt-scrnIndex, X_NOTICE,
+xf86DrvMsg(pInt-pScrn-scrnIndex, X_NOTICE,
Failing Find-Matching-File on non-PC
 (int 21, func 4e)\n);
 X86_AX = 2;
@@ -133,7 +133,7 @@ run_bios_int(int num, xf86Int10InfoPtr pInt)
 return 1;
 }
 else {
-xf86DrvMsgVerb(pInt-scrnIndex, X_NOT_IMPLEMENTED, 2,
+xf86DrvMsgVerb(pInt-pScrn-scrnIndex, X_NOT_IMPLEMENTED, 2,
Ignoring int 0x%02x call\n, num);
 if (xf86GetVerbosity()  3) {
 dump_registers(pInt);
@@ -169,7 +169,7 @@ dump_code(xf86Int10InfoPtr pInt)
 int i;
 CARD32 lina = SEG_ADR((CARD32), X86_CS, IP);
 
-xf86DrvMsgVerb(pInt-scrnIndex, X_INFO, 3, code at 0x%8.8 PRIx32 :\n,
+xf86DrvMsgVerb(pInt-pScrn-scrnIndex, X_INFO, 3, code at 0x%8.8 PRIx32 
:\n,
lina);
 for (i = 0; i  0x10; i++)
 xf86ErrorFVerb(3,  %2.2x, MEM_RB(pInt, lina + i));
@@ -182,19 +182,19 @@ dump_code(xf86Int10InfoPtr pInt)
 void
 dump_registers(xf86Int10InfoPtr pInt)
 {
-xf86DrvMsgVerb(pInt-scrnIndex, X_INFO, 3,
+xf86DrvMsgVerb(pInt-pScrn-scrnIndex, X_INFO, 3,
EAX=0x%8.8lx, EBX=0x%8.8lx, ECX=0x%8.8lx, EDX=0x%8.8lx\n,
(unsigned long) X86_EAX, (unsigned long) X86_EBX,
(unsigned long) X86_ECX, (unsigned long) X86_EDX);
-xf86DrvMsgVerb(pInt-scrnIndex, X_INFO, 3,
+xf86DrvMsgVerb(pInt-pScrn-scrnIndex, X_INFO, 3,
ESP=0x%8.8lx, EBP=0x%8.8lx, ESI=0x%8.8lx, EDI=0x%8.8lx\n,
(unsigned long) X86_ESP, (unsigned long) X86_EBP,
(unsigned long) X86_ESI, (unsigned long) X86_EDI);
-xf86DrvMsgVerb(pInt-scrnIndex, X_INFO, 3,
+xf86DrvMsgVerb(pInt-pScrn-scrnIndex, X_INFO, 3,
CS=0x%4.4x, SS=0x%4.4x,
 DS=0x%4.4x, ES=0x%4.4x, FS=0x%4.4x, GS=0x%4.4x\n,
X86_CS, X86_SS, X86_DS, X86_ES, X86_FS, X86_GS);
-xf86DrvMsgVerb(pInt-scrnIndex, X_INFO, 3,
+xf86DrvMsgVerb(pInt-pScrn-scrnIndex, X_INFO, 3,
EIP=0x%8.8lx, EFLAGS=0x%8.8lx\n,
(unsigned long) X86_EIP, (unsigned long) X86_EFLAGS);
 }
@@ -337,7 +337,7 @@ x_inb(CARD16 port)
 }
 else if (port  0x0100) {   /* Don't interfere with mainboard */
 val = 0;
-xf86DrvMsgVerb(Int10Current-scrnIndex, X_NOT_IMPLEMENTED, 2,
+xf86DrvMsgVerb(Int10Current-pScrn-scrnIndex, X_NOT_IMPLEMENTED, 2,
inb 0x%4.4x\n, port);
 if (xf86GetVerbosity()  3) {
 dump_registers(Int10Current);
@@ -395,7 +395,7 @@ x_outb(CARD16 port, CARD8 val)
 #ifdef __NOT_YET__
 }
 else if (port  0x0100) {   /* Don't interfere with mainboard */
-xf86DrvMsgVerb(Int10Current-scrnIndex, X_NOT_IMPLEMENTED, 2,
+xf86DrvMsgVerb(Int10Current-pScrn-scrnIndex, X_NOT_IMPLEMENTED, 2,
outb 0x%4.4x,0x%2.2x\n, port, val);
 if (xf86GetVerbosity()  3) {
 dump_registers(Int10Current);
diff --git a/hw/xfree86/int10/helper_mem.c b/hw/xfree86/int10/helper_mem.c
index 96c598a..160c5ae 100644
--- a/hw/xfree86/int10/helper_mem.c
+++ b/hw/xfree86/int10/helper_mem.c
@@ -281,7 +281,7 @@ xf86int10GetBiosLocationType(const xf86Int10InfoPtr pInt)
 
 #define CHECK_V_SEGMENT_RANGE(x)   \

Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)

2012-05-15 Thread Adam Jackson 

On 5/15/12 2:52 PM, Dave Airlie wrote:

Pass the ScrnInfoPtr instead of the index in the int10 struct.


Mostly good, but...


@@ -340,7 +341,7 @@ vbeDoEDID(vbeInfoPtr pVbe, pointer pDDCModule)
  if (!DDC_data)
  return NULL;

-pMonitor = xf86InterpretEDID(pVbe-pInt10-scrnIndex, DDC_data);
+pMonitor = xf86InterpretEDID(pVbe-pInt10-pScrn-scrnIndex, DDC_data);


The callee here wants index-ptr conversion too, doesn't it?  I don't 
think I see that in subsequent patches.


- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel