| Message ID | 20201026131120.1068959-1-pgofman@codeweavers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [libdrm] xf86drm.c: Use integer logarithm. | expand |
I've applied this to libdrm master. Thanks, Dave. On Mon, 26 Oct 2020 at 23:27, Paul Gofman <pgofman@codeweavers.com> wrote: > > log() is affected by FP control word and can provide inaccurate result. > > Fixes Killer Instinct under Wine not being able to find AMD vulkan > device. > > Signed-off-by: Paul Gofman <pgofman@codeweavers.com> > --- > With the rounding mode the application sets (unsigned int)log2(4) is 1. > The log2_int() implemetation is copied from radeon/radeon_surface.c. > > xf86drm.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/xf86drm.c b/xf86drm.c > index 50a6f092..dbb7c14b 100644 > --- a/xf86drm.c > +++ b/xf86drm.c > @@ -124,6 +124,22 @@ static drmServerInfoPtr drm_server_info; > static bool drmNodeIsDRM(int maj, int min); > static char *drmGetMinorNameForFD(int fd, int type); > > +static unsigned log2_int(unsigned x) > +{ > + unsigned l; > + > + if (x < 2) { > + return 0; > + } > + for (l = 2; ; l++) { > + if ((unsigned)(1 << l) > x) { > + return l - 1; > + } > + } > + return 0; > +} > + > + > drm_public void drmSetServerInfo(drmServerInfoPtr info) > { > drm_server_info = info; > @@ -4001,7 +4017,7 @@ static void drmFoldDuplicatedDevices(drmDevicePtr local_devices[], int count) > for (j = i + 1; j < count; j++) { > if (drmDevicesEqual(local_devices[i], local_devices[j])) { > local_devices[i]->available_nodes |= local_devices[j]->available_nodes; > - node_type = log2(local_devices[j]->available_nodes); > + node_type = log2_int(local_devices[j]->available_nodes); > memcpy(local_devices[i]->nodes[node_type], > local_devices[j]->nodes[node_type], drmGetMaxNodeName()); > drmFreeDevice(&local_devices[j]); > -- > 2.26.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, 26 Oct 2020 16:11:20 +0300 Paul Gofman <pgofman@codeweavers.com> wrote: > log() is affected by FP control word and can provide inaccurate result. > > Fixes Killer Instinct under Wine not being able to find AMD vulkan > device. > > Signed-off-by: Paul Gofman <pgofman@codeweavers.com> > --- > With the rounding mode the application sets (unsigned int)log2(4) is 1. > The log2_int() implemetation is copied from radeon/radeon_surface.c. > > xf86drm.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/xf86drm.c b/xf86drm.c > index 50a6f092..dbb7c14b 100644 > --- a/xf86drm.c > +++ b/xf86drm.c > @@ -124,6 +124,22 @@ static drmServerInfoPtr drm_server_info; > static bool drmNodeIsDRM(int maj, int min); > static char *drmGetMinorNameForFD(int fd, int type); > > +static unsigned log2_int(unsigned x) > +{ > + unsigned l; > + > + if (x < 2) { > + return 0; > + } > + for (l = 2; ; l++) { > + if ((unsigned)(1 << l) > x) { Hi, wouldn't this loop fail to end when x >= 0x80000000? Sure, such value probably cannot occur where this is currently used, but it seems like a landmine for the next developer to step on. Thanks, pq > + return l - 1; > + } > + } > + return 0; > +} > + > + > drm_public void drmSetServerInfo(drmServerInfoPtr info) > { > drm_server_info = info; > @@ -4001,7 +4017,7 @@ static void drmFoldDuplicatedDevices(drmDevicePtr local_devices[], int count) > for (j = i + 1; j < count; j++) { > if (drmDevicesEqual(local_devices[i], local_devices[j])) { > local_devices[i]->available_nodes |= local_devices[j]->available_nodes; > - node_type = log2(local_devices[j]->available_nodes); > + node_type = log2_int(local_devices[j]->available_nodes); > memcpy(local_devices[i]->nodes[node_type], > local_devices[j]->nodes[node_type], drmGetMaxNodeName()); > drmFreeDevice(&local_devices[j]);
On 10/28/20 11:18, Pekka Paalanen wrote: > >> >> +static unsigned log2_int(unsigned x) >> +{ >> + unsigned l; >> + >> + if (x < 2) { >> + return 0; >> + } >> + for (l = 2; ; l++) { >> + if ((unsigned)(1 << l) > x) { > Hi, > > wouldn't this loop fail to end when x >= 0x80000000? > > Sure, such value probably cannot occur where this is currently used, > but it seems like a landmine for the next developer to step on. > Indeed, thanks. I've sent the patches for consideration which avoid function duplication and potentially infinite loop.
On 2020-10-28 11:09 a.m., Paul Gofman wrote: > On 10/28/20 11:18, Pekka Paalanen wrote: >> >>> >>> +static unsigned log2_int(unsigned x) >>> +{ >>> + unsigned l; >>> + >>> + if (x < 2) { >>> + return 0; >>> + } >>> + for (l = 2; ; l++) { >>> + if ((unsigned)(1 << l) > x) { >> Hi, >> >> wouldn't this loop fail to end when x >= 0x80000000? >> >> Sure, such value probably cannot occur where this is currently used, >> but it seems like a landmine for the next developer to step on. >> > Indeed, thanks. I've sent the patches for consideration which avoid > function duplication and potentially infinite loop. libdrm uses GitLab merge requests now: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests
On 10/28/20 13:30, Michel Dänzer wrote: > On 2020-10-28 11:09 a.m., Paul Gofman wrote: >> On 10/28/20 11:18, Pekka Paalanen wrote: >>> >>>> +static unsigned log2_int(unsigned x) >>>> +{ >>>> + unsigned l; >>>> + >>>> + if (x < 2) { >>>> + return 0; >>>> + } >>>> + for (l = 2; ; l++) { >>>> + if ((unsigned)(1 << l) > x) { >>> Hi, >>> >>> wouldn't this loop fail to end when x >= 0x80000000? >>> >>> Sure, such value probably cannot occur where this is currently used, >>> but it seems like a landmine for the next developer to step on. >>> >> Indeed, thanks. I've sent the patches for consideration which avoid >> function duplication and potentially infinite loop. > > libdrm uses GitLab merge requests now: > https://gitlab.freedesktop.org/mesa/drm/-/merge_requests > > I see, thanks. I was following the instructions in CONTRIBUTING.rst. Do you think I should proceed with merge request for these patches or those already sent could be considered this way?
diff --git a/xf86drm.c b/xf86drm.c index 50a6f092..dbb7c14b 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -124,6 +124,22 @@ static drmServerInfoPtr drm_server_info; static bool drmNodeIsDRM(int maj, int min); static char *drmGetMinorNameForFD(int fd, int type); +static unsigned log2_int(unsigned x) +{ + unsigned l; + + if (x < 2) { + return 0; + } + for (l = 2; ; l++) { + if ((unsigned)(1 << l) > x) { + return l - 1; + } + } + return 0; +} + + drm_public void drmSetServerInfo(drmServerInfoPtr info) { drm_server_info = info; @@ -4001,7 +4017,7 @@ static void drmFoldDuplicatedDevices(drmDevicePtr local_devices[], int count) for (j = i + 1; j < count; j++) { if (drmDevicesEqual(local_devices[i], local_devices[j])) { local_devices[i]->available_nodes |= local_devices[j]->available_nodes; - node_type = log2(local_devices[j]->available_nodes); + node_type = log2_int(local_devices[j]->available_nodes); memcpy(local_devices[i]->nodes[node_type], local_devices[j]->nodes[node_type], drmGetMaxNodeName()); drmFreeDevice(&local_devices[j]);
log() is affected by FP control word and can provide inaccurate result. Fixes Killer Instinct under Wine not being able to find AMD vulkan device. Signed-off-by: Paul Gofman <pgofman@codeweavers.com> --- With the rounding mode the application sets (unsigned int)log2(4) is 1. The log2_int() implemetation is copied from radeon/radeon_surface.c. xf86drm.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)