Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)
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 Reviewed-by: Alan Coopersmith -- -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)
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)
> Date: Wed, 16 May 2012 10:31:26 +0100 > From: Dave Airlie > > On Wed, May 16, 2012 at 10:24 AM, Mark Kettenis > wrote: > >> Date: Wed, 16 May 2012 10:16:24 +0100 > >> From: 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. > > > > 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
Re: [PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)
On Wed, May 16, 2012 at 10:24 AM, Mark Kettenis wrote: >> Date: Wed, 16 May 2012 10:16:24 +0100 >> From: 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. > > 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)
> Date: Wed, 16 May 2012 10:16:24 +0100 > From: 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. 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)
> >> @@ -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)
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
[PATCH] int10/vbe: don't use xf86Screens. (ABI) (v2)
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
---
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 xf86Int10
