headers
Bob Gleitsmann | 1 Jan 2011 22:35

Proper implemtation of glFinish

Hello everyone,

When trying the demo program copytex for the first time recently, I noticed 
pathological behavior: after running for a long time it asserted out and 
locked up X. Investigation showed this to be due to the glFinish function 
acting like glFlush and not waiting as it is supposed to for completion of 
whatever commands had been issued. I took up the task of remedying this. There 
are, as usual, a variety of different ways of doing so. Influenced by the 
current gallium code, I planned a separate call to the kernel to wait for the 
fence created by the pushbuf flush ioctl to complete. After completing the 
implementation in this way, it occurred to me that it would be more economical 
to modify the pushbuf flush ioctl call with a flag to indicate whether it should 
wait for completion or not. This would require modifying the FIRE_RING inline 
which appears in numerous places. Perhaps my original plan is adequate. 
The code changes required for the original plan involve mesa, drm, and the 
kernel. This forum seems like the best starting point to solicit feedback, 
since the whole thing is motivated by mesa. The Xorg nouveau driver does not 
seem to need anything of this nature. I have tested on my current hardware, 
x86-64 dual opteron and 7300 GT card. 

The assert out noted in the beginning was due to timeout in the 
__nouveau_fence_wait function in the kernel. Unfortunately, it does not exit 
gracefully. The patches that I came up with eliminate the timeout in the case 
of copytex, but this is clearly a flaw that someday should have a more complete 
fix. 
Please let me know if I should send the kernel and drm patches to another 
forum (like dri-devel). For now I will post them here. 

Best Wishes,
(Continue reading)

Permalink | Reply | 4 comments |
headers
Bob Gleitsmann | 1 Jan 2011 22:47

glFinish Patch 1 of 3 - kernel


diff --git a/drivers/gpu/drm/nouveau/nouveau_channel.c 
b/drivers/gpu/drm/nouveau/nouveau_channel.c
index a3d33a5..ac2fe5a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_channel.c
+++ b/drivers/gpu/drm/nouveau/nouveau_channel.c
 <at>  <at>  -466,6 +466,35  <at>  <at>  nouveau_ioctl_fifo_free(struct drm_device *dev, void 
*data,
 	return 0;
 }

+int
+nouveau_ioctl_fifo_wait(struct drm_device *dev, void *data,
+		struct drm_file *file_priv)
+{
+	struct nouveau_fence *fence = NULL;
+	struct drm_nouveau_channel_wait *req = data;
+	struct nouveau_channel *chan;
+	int ret = 0;
+
+	chan = nouveau_channel_get(dev, file_priv, req->channel);
+	if (IS_ERR(chan))
+		return PTR_ERR(chan);
+
+	nouveau_fence_update(chan);
+
+	if (chan->fence.sequence != chan->fence.sequence_ack) {
+		ret = nouveau_fence_new(chan, &fence, true);
+		if (!ret) {
+			ret = nouveau_fence_wait(fence, true, true);
(Continue reading)

Permalink | Reply | 0 comments |
headers
Bob Gleitsmann | 1 Jan 2011 22:49

glFinish Patch 2 of 3 - drm


diff --git a/include/drm/nouveau_drm.h b/include/drm/nouveau_drm.h
index b18cad0..d2ce47e 100644
--- a/include/drm/nouveau_drm.h
+++ b/include/drm/nouveau_drm.h
 <at>  <at>  -49,6 +49,10  <at>  <at>  struct drm_nouveau_channel_free {
 	int channel;
 };

+struct drm_nouveau_channel_wait {
+	int channel;
+};
+
 struct drm_nouveau_grobj_alloc {
 	int      channel;
 	uint32_t handle;
 <at>  <at>  -204,5 +208,6  <at>  <at>  struct drm_nouveau_sarea {
 #define DRM_NOUVEAU_GEM_CPU_PREP       0x42
 #define DRM_NOUVEAU_GEM_CPU_FINI       0x43
 #define DRM_NOUVEAU_GEM_INFO           0x44
+#define DRM_NOUVEAU_CHANNEL_WAIT       0x45

 #endif /* __NOUVEAU_DRM_H__ */
diff --git a/nouveau/nouveau_channel.c b/nouveau/nouveau_channel.c
index 96fa03b..1f03732 100644
--- a/nouveau/nouveau_channel.c
+++ b/nouveau/nouveau_channel.c
 <at>  <at>  -139,4 +139,20  <at>  <at>  nouveau_channel_free(struct nouveau_channel **chan)
 	free(nvchan);
 }
(Continue reading)

Permalink | Reply | 0 comments |
headers
Bob Gleitsmann | 1 Jan 2011 22:52

glFinish Patch 3 of 3 - Mesa


diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c 
b/src/gallium/drivers/nouveau/nouveau_screen.c
index a9426df..0918537 100644
--- a/src/gallium/drivers/nouveau/nouveau_screen.c
+++ b/src/gallium/drivers/nouveau/nouveau_screen.c
 <at>  <at>  -166,7 +166,8  <at>  <at>  nouveau_screen_fence_finish(struct pipe_screen *screen,
 			    struct pipe_fence_handle *pfence,
 			    unsigned flags)
 {
-	return 0;
+	struct nouveau_screen *ns = nouveau_screen(screen);
+	return nouveau_channel_wait(ns->channel);
 }

 
diff --git a/src/gallium/drivers/nvfx/nvfx_context.c 
b/src/gallium/drivers/nvfx/nvfx_context.c
index 6c8934d..046ad84 100644
--- a/src/gallium/drivers/nvfx/nvfx_context.c
+++ b/src/gallium/drivers/nvfx/nvfx_context.c
 <at>  <at>  -24,8 +24,11  <at>  <at>  nvfx_flush(struct pipe_context *pipe, unsigned flags,
 	}

 	FIRE_RING(chan);
+/* Gallium only checks if *fence is non-zero on return. fence here is null 
when called
+ * from glFlush, non-null when called from glFinish. The real fence lives 
only in the kernel.
+ */
(Continue reading)

Permalink | Reply | 0 comments |
headers
Dave Airlie | 3 Jan 2011 11:39
Picon
Gravatar

GLX_EXT_framebuffer_sRGB support

So I've been looking at the GLX support and getting DRI/GLX support
for this running.

Just not sure how best to get it merged.

http://people.freedesktop.org/~airlied/framebuffer_srgb/

the first patch is one to glproto adding the token to glxtokens.h

then there is some mesa hacks which add the DRI support also to dri_interface.h,
along with adding a new boolean to the driCreateConfigs which the
driver can ask for the attrib to be set
(plan is to set it for 888 and 8888 formats as per the suggestions in
the EXT_framebuffer_sRGB spec).

then there is an X server patch which adds the GLX/DRI support,
however it needs the DRI_ATTRIB defines from the mesa patch.

Just wondering if we have a best practice for merging something like
this, I haven't even really gotten to the driver side yet, just trying
to get GLX/DRI framework in place first.

Dave.
Permalink | Reply | 0 comments |
headers
Brian Paul | 3 Jan 2011 16:43
Favicon

Re: Gallium improvements and cleanups

On 12/29/2010 11:59 AM, Marek Olšák wrote:
> Hi,
>
> first there's a few of patches of mine that are still awaiting a review. Those are:
>
> [PATCH] st/mesa: advertise GL_ARB_half_float_pixel (posted on Dec 18)
> [PATCH] mesa: preserve 10 bits of precision in the texstore general path for ARGB2101010 (posted on Dec 22)
> [PATCH] u_upload_mgr: keep the upload buffer mapped until it is flushed (posted on Dec 25)
>
>
> Now about this patch series. It contains all sorts of stuff from my private branch which I consider useful.
Please see below.
>
> [PATCH 1/7] u_upload_mgr: new features
> - This is an important patch I need for r300g, because the current u_upload_mgr interface no longer suits
my needs. It depends on the previous u_upload_mgr patch sent on Dec 25.
>
> [PATCH 2/7] st/mesa: do sanity checks on states only in debug builds
> - This is a little performance improvement.
>
> [PATCH 3/7] st/mesa: optimize constant buffer uploads
> - This is an important performance improvement and gives drivers more freedom as to how they implement
constant buffer uploads.
>
> [PATCH 4/7] gallium: drivers should reference vertex buffers
> - This is a preparation for atomizing vertex arrays in st/mesa, i.e. something that we need to address our
biggest performance issues on the state tracker side. I'll post more on that once I have something that
works and isn't a set of ugly hacks.
>
> [PATCH 5/7] tgsi: remove redundant name tables from tgsi_text, use those from tgsi_dump
(Continue reading)

Permalink | Reply | 0 comments |
headers
Brian Paul | 3 Jan 2011 16:43
Favicon

Re: [PATCH 1/7] u_upload_mgr: new features

Could you also add comments to all the u_upload_mgr.c functions?  In 
particular, what are the possible values for the usage/bind 
parameters?  A follow-on patch is fine.  I know that comments have 
been missing all along.

-Brian

On 12/29/2010 12:00 PM, Marek Olšák wrote:
> - Added a parameter to specify a minimum offset that should be returned.
>    r300g needs this to better implement user buffer uploads. This weird
>    requirement comes from the fact that the Radeon DRM doesn't support negative
>    offsets.
>
> - Added a parameter to notify a driver that the upload flush occured.
>    A driver may skip buffer validation if there was no flush, resulting
>    in a better performance.
>
> - Added a new upload function that returns a pointer to the upload buffer
>    directly, so that the buffer can be filled e.g. by the translate module.
> ---
>   src/gallium/auxiliary/util/u_upload_mgr.c     |   75 +++++++++++++++++--------
>   src/gallium/auxiliary/util/u_upload_mgr.h     |   43 +++++++++++++-
>   src/gallium/drivers/i965/brw_draw_upload.c    |   12 +++-
>   src/gallium/drivers/r300/r300_screen_buffer.c |   10 ++-
>   src/gallium/drivers/svga/svga_draw_elements.c |    5 +-
>   src/gallium/drivers/svga/svga_state_vdecl.c   |    6 +-
>   6 files changed, 115 insertions(+), 36 deletions(-)
>
> diff --git a/src/gallium/auxiliary/util/u_upload_mgr.c b/src/gallium/auxiliary/util/u_upload_mgr.c
> index 4d97bf0..80c9b63 100644
(Continue reading)

Permalink | Reply | 2 comments |
headers
Brian Paul | 3 Jan 2011 16:43
Favicon

Re: [PATCH 4/7] gallium: drivers should reference vertex buffers

On 12/29/2010 12:00 PM, Marek Olšák wrote:

This function should have a comment explaining exactly what it's doing:

> +static INLINE void util_copy_vertex_buffers(struct pipe_vertex_buffer *dst,
> +                                            unsigned *dst_count,
> +                                            const struct pipe_vertex_buffer *src,
> +                                            unsigned src_count)
> +{
> +   unsigned i;
> +
> +   for (i = 0; i<  src_count; i++) {
> +      pipe_resource_reference(&dst[i].buffer, src[i].buffer);
> +   }
> +   for (; i<  *dst_count; i++) {
> +      pipe_resource_reference(&dst[i].buffer, NULL);
> +   }
> +
> +   *dst_count = src_count;
> +   memcpy(dst, src, src_count * sizeof(struct pipe_vertex_buffer));
> +}
> +
>   #ifdef __cplusplus
>   }
>   #endif
Permalink | Reply | 0 comments |
headers
Brian Paul | 3 Jan 2011 16:47
Picon

Re: [PATCH] mesa: preserve 10 bits of precision in the texstore general path for ARGB2101010

Looks good.

-Brian

On Wed, Dec 22, 2010 at 12:50 PM, Marek Olšák <maraeo <at> gmail.com> wrote:
> Use make_temp_float_image instead of _make_temp_chan_image.
> The latter converts the texture to 8 bits/component, losing 2 bits.
>
> This is a follow-up to my last patch series.
> ---
>  src/mesa/main/colormac.h     |    6 +++++-
>  src/mesa/main/texfetch_tmp.h |    2 +-
>  src/mesa/main/texstore.c     |   32 +++++++++++++++++++-------------
>  3 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/src/mesa/main/colormac.h b/src/mesa/main/colormac.h
> index 065f9f9..a328dcd 100644
> --- a/src/mesa/main/colormac.h
> +++ b/src/mesa/main/colormac.h
>  <at>  <at>  -198,10 +198,14  <at>  <at>  do {                                              \
>    ((((B) & 0xf8) >> 1) | (((G) & 0xc0) >> 6) | (((G) & 0x38) << 10) | (((R) & 0xf8) << 5) |   \
>     ((A) ? 0x80 : 0))
>
> -#define PACK_COLOR_2101010( A, B, G, R )                                       \
> +#define PACK_COLOR_2101010_UB( A, B, G, R )                                    \
>    (((B) << 22) | ((G) << 12) | ((R) << 2) |   \
>     (((A) & 0xc0) << 24))
>
> +#define PACK_COLOR_2101010_US( A, B, G, R )                                    \
> +   ((((B) >> 6) << 20) | (((G) >> 6) << 10) | ((R) >> 6) |     \
(Continue reading)

Permalink | Reply | 0 comments |
headers
bugzilla-daemon | 3 Jan 2011 22:02

[Bug 30694] wincopy will crash on Gallium drivers when going to front buffer

https://bugs.freedesktop.org/show_bug.cgi?id=30694

Brian Paul <brianp <at> vmware.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #8 from Brian Paul <brianp <at> vmware.com> 2011-01-03 13:02:16 PST ---
Fixed with commit efbd33aff93d875af95d40db018b7911a3f87d02

--

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
Permalink | Reply | 0 comments |

Gmane