Re: [Mesa-dev] [PATCH 03/23] i965: Separate miptree creation from auxiliary buffer setup
On Tue, Feb 09, 2016 at 03:05:05PM -0800, Ben Widawsky wrote:
> On Mon, Feb 08, 2016 at 06:51:23PM +0200, Topi Pohjolainen wrote:
> > Currently the logic allocating and setting up miptrees is closely
> > combined with decision making when to re-allocate buffers in
> > X-tiled layout and when to associate colors with auxiliary buffers.
> >
> > These auxiliary buffers are in turn also represented as miptrees
> > and are created by the same miptree creation logic calling itself
> > recursively. This means considering in vain if the auxiliary buffers
> > should be represented in X-tiled layout or if they should be
> > associated with auxiliary buffers again.
> > While this is somewhat unnecessary, this doesn't impose any problems
> > currently. Miptrees for auxiliary buffers are created as simgle-sampled
> > fusing the consideration for multi-sampled compression auxiliary
> > buffers. The format in turn is such that is not applicable for
> > single-sampled fast clears (that would require accompaning auxiliary
> > buffer).
> > But once the driver starts to support lossless compression of color
> > buffers the auxiliary buffer will have a format that would itself
> > be applicable for lossless compression. This would be rather
> > difficult and ugly to detect in the current miptree creation logic,
> > and therefore this patch seeks to separate the association logic
> > from the general allocation and setup steps.
> >
> > Signed-off-by: Topi Pohjolainen
>
> Shameless plug. I think we reached the same conclusion about the existing code
> :-). I forgot what I actually did, but I'm pretty sure I at least did this
> stuff, and maybe some more. I'd love it if you could see if anything is useful
> there. https://patchwork.freedesktop.org/patch/56792/
Right, the logic below reconsiders TILING_X even though the caller overwrote
it to TILING_Y. Your patch addresses this, I will update this accordingly.
>
> > ---
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 75
> > +--
> > 1 file changed, 49 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 033f4c6..d655de8 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -611,17 +611,18 @@ intel_get_yf_ys_bo_size(struct intel_mipmap_tree *mt,
> > unsigned *alignment,
> > return size;
> > }
> >
> > -struct intel_mipmap_tree *
> > -intel_miptree_create(struct brw_context *brw,
> > - GLenum target,
> > - mesa_format format,
> > - GLuint first_level,
> > - GLuint last_level,
> > - GLuint width0,
> > - GLuint height0,
> > - GLuint depth0,
> > - GLuint num_samples,
> > - uint32_t layout_flags)
> > +static struct intel_mipmap_tree *
> > +miptree_create(struct brw_context *brw,
> > + GLenum target,
> > + mesa_format format,
> > + GLuint first_level,
> > + GLuint last_level,
> > + GLuint width0,
> > + GLuint height0,
> > + GLuint depth0,
> > + GLuint num_samples,
> > + enum intel_msaa_layout msaa_layout,
> > + uint32_t layout_flags)
> > {
> > struct intel_mipmap_tree *mt;
> > mesa_format tex_format = format;
> > @@ -638,9 +639,7 @@ intel_miptree_create(struct brw_context *brw,
> > mt = intel_miptree_create_layout(brw, target, format,
> > first_level, last_level, width0,
> > height0, depth0, num_samples,
> > -compute_msaa_layout(brw, format,
> > -num_samples,
> > false),
> > -layout_flags);
> > +msaa_layout, layout_flags);
> > /*
> > * pitch == 0 || height == 0 indicates the null texture
> > */
> > @@ -658,12 +657,8 @@ intel_miptree_create(struct brw_context *brw,
> >total_height = ALIGN(total_height, 64);
> > }
> >
> > - bool y_or_x = false;
> > -
> > - if (mt->tiling == (I915_TILING_Y | I915_TILING_X)) {
> > - y_or_x = true;
> > + if (mt->tiling == (I915_TILING_Y | I915_TILING_X))
> >mt->tiling = I915_TILING_Y;
> > - }
> >
> > if (layout_flags & MIPTREE_LAYOUT_ACCELERATED_UPLOAD)
> >alloc_flags |= BO_ALLOC_FOR_RENDER;
> > @@ -686,12 +681,46 @@ intel_miptree_create(struct brw_context *brw,
> > }
> >
> > mt->pitch = pitch;
> > + mt->offset = 0;
> > +
> > + if (!mt->bo) {
> > + intel_miptree_release(&mt);
> > + return NULL;
> > + }
> > +
> > + return mt;
> > +}
> > +
> > +struct intel_mipmap_tree *
> > +intel_miptr
Re: [Mesa-dev] [PATCH 0/2] Simple Klocwork patches
Hi Juha, On Sun, 2016-02-07 at 23:37 +0200, Juha-Pekka Heikkilä wrote: > Hi Iago, > > I know there are lot of places where there is malloc unchecked still > -- and then there is ralloc which is a story of its own. Reason why I > think checking these would be remotely useful in windows only (or > other way around, not under linux kernel) is on Windows one can get > the null pointer from malloc. On Androids I think memory over > committing has always been enabled and on Linux I suspect I belong to > the minority who like to set ulimits for memory. > > I agree checking these mostly is quite useless but there are those > corners where it may suddenly become valuable. When process is running > and everything has settled it will be weird if hit any of these checks > but any code which is run when process is starting I notice is the > place where things will fail if they fail. This is of course just my > opinion about the value of these checks but I really dislike > possibility of segfault when it is coming from a library. > > I didn't quickly notice where _mesa_error() get more heap. Stack it of > course needs but when I did stress test these _mesa_error() did still > work. Cannot promise my test was 100% correct though, I think it was > over year ago when I was playing with it. > I think Matt explained this but I suppose it might still help in some platforms. Ian's point about using NULL as an offset is also valid. Anyway, I am not blocking anything, the patches are a small thing and they might help in some cases so feel to go ahead with them. If you still need a Reviewed-by for these you can add mine or Samuel's. Iago > > On Wed, Feb 3, 2016 at 5:12 PM, Iago Toral wrote: > > Hi Juha, > > > > I don't know why checking for this might be more relevant in Windows, > > but in any case: > > > > There are a ton of other places in mesa where we allocate memory via > > calloc/malloc and we don't check that the allocation actually succeeded > > so I am not sure that fixing a couple of instances of *small* > > allocations changes anything. > > > > IMHO, this kind of things are only really useful when allocating memory > > for large amounts of data, otherwise even if you check for a NULL > > allocation you still need to make sure that you don't need any extra > > memory to handle that situation, and _mesa_error() needs memory, so it > > is probably not really giving us anything in practice other than > > silencing Klocwork... > > > > Iago > > > > On Wed, 2016-02-03 at 10:56 +0200, Juha-Pekka Heikkila wrote: > >> I'm thinking these things maybe could be wrapped up inside something like > >> "#ifdef windows" or so in the future. At least for Android and Linux these > >> are normally quite useless. > >> > >> /Juha-Pekka > >> > >> Juha-Pekka Heikkila (2): > >> i965: in brw_link_shader() react to low memory > >> glsl: Check for null pointer at ir_variable_refcount_visitor() > >> > >> src/compiler/glsl/ir_variable_refcount.cpp | 7 +++ > >> src/mesa/drivers/dri/i965/brw_link.cpp | 4 > >> src/mesa/main/ff_fragment_shader.cpp | 6 -- > >> 3 files changed, 15 insertions(+), 2 deletions(-) > >> > > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3] i965/blorp: Fix hiz ops on MSAA surfaces
On 09/02/16 02:29, Jordan Justen wrote: > On 2016-02-06 10:25:59, Ben Widawsky wrote: >> On Sat, Feb 06, 2016 at 12:01:50PM +0100, Alejandro Piñeiro wrote: >>> From: Chris Forbes >>> >>> Two things were broken here: >>> - The depth/stencil surface dimensions were broken for MSAA. >>> - Sample count was programmed incorrectly. >>> >>> Result was the depth resolve didn't work correctly on MSAA surfaces, and >>> so sampling the surface later produced garbage. >>> >>> Fixes the new piglit test arb_texture_multisample-sample-depth, and >>> various artifacts in 'tesseract' with msaa=4 glineardepth=0. >>> >>> Fixes freedesktop bug #76396. >>> >>> Not observed any piglit regressions on Haswell. >>> >>> v2: Just set brw_hiz_op_params::dst.num_samples rather than adding a >>> helper function (Ken). >>> >>> v3: moved the alignment needed for hiz+msaa to brw_blorp.cpp, as >>> suggested by Chad Versace (Alejandro Piñeiro on behalf of Chris >>> Forbes) >>> --- >>> >>> While taking a look to bug #76396, I found that the git commit message >>> on arb_texture_multisample/sample-depth.c mentions that MSAA+hiz not >>> working on Haswell was an already know bug. After pinging on IRC Ben >>> Widawsky pointed me this around one year old review thread. >>> >>> It seems that the original patch got not updated after Chad Versace >>> review. Hoping to not stomp on any toe, I made the update myself (so >>> this v3 update). Even if this update is not accepted, hopefully this >>> will resume the review process and the bug will be fixed soon. >>> >>> I re-run piglit on Haswell, so "Not observed any piglit regressions on >>> Haswell." still applies. >>> >>> I also slightly changed the commit message to reflect that this patch >>> fixes bug #76396. Also removed the "Signed-off-by: Chris Forbes >>> " because is not technically true anymore. Chris can >>> re-add that again when he push the patch, if accepted. >> Thanks for doing this Alejandro. Like Ilia said though, the common process >> is to >> just add your SoB on top if there was one there to begin with. >> > On top? I usually add to the bottom. That was also what Illia suggested. > Alejandro, > > I ran the patch through jenkins, and it seemed to fix Sandy Bridge, > Ivy Bridge and Haswell for > piglit.spec.arb_texture_multisample.arb_texture_multisample-sample-depth > > Tested-by: Jordan Justen > Reviewed-by: Jordan Justen Two reviews, so I think that it is safe to push without waiting for Chad Versace. I have just pushed the commit, adding the Signed-off as you all suggested. Thanks everybody for the patience and the reviews. BR ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
