Re: [Mesa-dev] [PATCH v2 1/9] panfrost: Make ctx->job useful
On Thu, 2019-08-08 at 15:28 +0200, Boris Brezillon wrote:
> On Thu, 8 Aug 2019 06:08:05 -0700
> Alyssa Rosenzweig wrote:
>
> > @Boris I don't see why this patch particularly needs to be backported to
> > 19.1?
>
> Nope, no reason to backport it to 19.1. It's just a (bad?) kernel dev
> habit to put "Fixes:" tag the patch fixes a problem introduced by a
> previous commit. But it does not necessarily means "backport to stable"
> in mind.
>
If a patch fixes another patch that is in the stable branch, we try at our best
to include the fix also in the stable branch. I think it makes sense to fix
something that is already in stable.
But if you think this fix is not worth to be on stable, that's fine. I'll keep
it ignored.
Thanks!
> > On Thu, Aug 08, 2019 at 11:46:43AM +0200, Juan A. Suarez Romero wrote:
> > > On Fri, 2019-08-02 at 19:18 +0200, Boris Brezillon wrote:
> > > > ctx->job is supposed to serve as a cache to avoid an hash table lookup
> > > > everytime we access the job attached to the currently bound FB, except
> > > > it was never assigned to anything but NULL.
> > > >
> > > > Fix that by adding the missing assignment in panfrost_get_job_for_fbo().
> > > > Also add a missing NULL assignment in the ->set_framebuffer_state()
> > > > path.
> > > >
> > > > While at it, add extra assert()s to make sure ctx->job is consistent.
> > > >
> > > > Fixes: 59c9623d0a75 ("panfrost: Import job data structures from v3d")
> > > > Signed-off-by: Boris Brezillon
> > > > Changes in v2:
> > > > * New patch (suggested by Alyssa)
> > >
> > > This patch is a candidate for 19.1, but unfortunately it does not apply
> > > cleanly.
> > > It seems it depends on a bunch of other commits not in 19.1
> > >
> > > At this point, I will ignore/reject this patch for 19.1, unless you can
> > > provide
> > > me a specific version for 19.1 branch (use staging/19.1 as reference).
> > >
> > > Thanks.
> > >
> > > J.A.
> > >
> > > > ---
> > > > src/gallium/drivers/panfrost/pan_context.c | 5 +
> > > > src/gallium/drivers/panfrost/pan_job.c | 19 ++-
> > > > 2 files changed, 23 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/gallium/drivers/panfrost/pan_context.c
> > > > b/src/gallium/drivers/panfrost/pan_context.c
> > > > index 014f8f6a9d07..ba2df2ce66ae 100644
> > > > --- a/src/gallium/drivers/panfrost/pan_context.c
> > > > +++ b/src/gallium/drivers/panfrost/pan_context.c
> > > > @@ -2372,6 +2372,11 @@ panfrost_set_framebuffer_state(struct
> > > > pipe_context *pctx,
> > > > panfrost_flush(pctx, NULL, PIPE_FLUSH_END_OF_FRAME);
> > > > }
> > > >
> > > > +/* Invalidate the FBO job cache since we've just been assigned
> > > > a new
> > > > + * FB state.
> > > > + */
> > > > +ctx->job = NULL;
> > > > +
> > > > util_copy_framebuffer_state(&ctx->pipe_framebuffer, fb);
> > > >
> > > > /* Given that we're rendering, we'd love to have compression */
> > > > diff --git a/src/gallium/drivers/panfrost/pan_job.c
> > > > b/src/gallium/drivers/panfrost/pan_job.c
> > > > index 6339b39d29a0..cc81d475165e 100644
> > > > --- a/src/gallium/drivers/panfrost/pan_job.c
> > > > +++ b/src/gallium/drivers/panfrost/pan_job.c
> > > > @@ -23,6 +23,8 @@
> > > > *
> > > > */
> > > >
> > > > +#include
> > > > +
> > > > #include "pan_context.h"
> > > > #include "util/hash_table.h"
> > > > #include "util/ralloc.h"
> > > > @@ -124,8 +126,13 @@ panfrost_get_job_for_fbo(struct panfrost_context
> > > > *ctx)
> > > >
> > > > /* If we already began rendering, use that */
> > > >
> > > > -if (ctx->job)
> > > > +if (ctx->job) {
> > > > +assert(ctx->job->key.zsbuf ==
> > > > ctx->pipe_framebuffer.zsbuf &&
> > > > + !memcmp(ctx->job->key.cbufs,
> > > > + ctx->pipe_framebuffer.cbufs,
> > > > + sizeof(ctx->job->key.cbufs)));
> > > > return ctx->job;
> > > > +}
> > > >
> > > > /* If not, look up the job */
> > > >
> > > > @@ -133,6 +140,10 @@ panfrost_get_job_for_fbo(struct panfrost_context
> > > > *ctx)
> > > > struct pipe_surface *zsbuf = ctx->pipe_framebuffer.zsbuf;
> > > > struct panfrost_job *job = panfrost_get_job(ctx, cbufs, zsbuf);
> > > >
> > > > +/* Set this job as the current FBO job. Will be reset when
> > > > updating the
> > > > + * FB state and when submitting or releasing a job.
> > > > + */
> > > > +ctx->job = job;
> > > > return job;
> > > > }
> > > >
> > > > @@ -181,6 +192,12 @@ panfrost_job_submit(struct panfrost_context *ctx,
> > > > struct panfrost_job *job)
> > > >
> > > > if (ret)
> > > > fprintf(stderr, "panfrost_job_submit failed: %d\n",
> > > > ret);
> > > > +
> > > > +/* The job has been submitted, let's invalidate the current
> > > > F
Re: [Mesa-dev] [PATCH v2 1/9] panfrost: Make ctx->job useful
On Thu, 8 Aug 2019 06:08:05 -0700
Alyssa Rosenzweig wrote:
> @Boris I don't see why this patch particularly needs to be backported to
> 19.1?
Nope, no reason to backport it to 19.1. It's just a (bad?) kernel dev
habit to put "Fixes:" tag the patch fixes a problem introduced by a
previous commit. But it does not necessarily means "backport to stable"
in mind.
>
> On Thu, Aug 08, 2019 at 11:46:43AM +0200, Juan A. Suarez Romero wrote:
> > On Fri, 2019-08-02 at 19:18 +0200, Boris Brezillon wrote:
> > > ctx->job is supposed to serve as a cache to avoid an hash table lookup
> > > everytime we access the job attached to the currently bound FB, except
> > > it was never assigned to anything but NULL.
> > >
> > > Fix that by adding the missing assignment in panfrost_get_job_for_fbo().
> > > Also add a missing NULL assignment in the ->set_framebuffer_state()
> > > path.
> > >
> > > While at it, add extra assert()s to make sure ctx->job is consistent.
> > >
> > > Fixes: 59c9623d0a75 ("panfrost: Import job data structures from v3d")
> > > Signed-off-by: Boris Brezillon
> > > Changes in v2:
> > > * New patch (suggested by Alyssa)
> >
> >
> > This patch is a candidate for 19.1, but unfortunately it does not apply
> > cleanly.
> > It seems it depends on a bunch of other commits not in 19.1
> >
> > At this point, I will ignore/reject this patch for 19.1, unless you can
> > provide
> > me a specific version for 19.1 branch (use staging/19.1 as reference).
> >
> > Thanks.
> >
> > J.A.
> >
> > > ---
> > > src/gallium/drivers/panfrost/pan_context.c | 5 +
> > > src/gallium/drivers/panfrost/pan_job.c | 19 ++-
> > > 2 files changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/gallium/drivers/panfrost/pan_context.c
> > > b/src/gallium/drivers/panfrost/pan_context.c
> > > index 014f8f6a9d07..ba2df2ce66ae 100644
> > > --- a/src/gallium/drivers/panfrost/pan_context.c
> > > +++ b/src/gallium/drivers/panfrost/pan_context.c
> > > @@ -2372,6 +2372,11 @@ panfrost_set_framebuffer_state(struct pipe_context
> > > *pctx,
> > > panfrost_flush(pctx, NULL, PIPE_FLUSH_END_OF_FRAME);
> > > }
> > >
> > > +/* Invalidate the FBO job cache since we've just been assigned a
> > > new
> > > + * FB state.
> > > + */
> > > +ctx->job = NULL;
> > > +
> > > util_copy_framebuffer_state(&ctx->pipe_framebuffer, fb);
> > >
> > > /* Given that we're rendering, we'd love to have compression */
> > > diff --git a/src/gallium/drivers/panfrost/pan_job.c
> > > b/src/gallium/drivers/panfrost/pan_job.c
> > > index 6339b39d29a0..cc81d475165e 100644
> > > --- a/src/gallium/drivers/panfrost/pan_job.c
> > > +++ b/src/gallium/drivers/panfrost/pan_job.c
> > > @@ -23,6 +23,8 @@
> > > *
> > > */
> > >
> > > +#include
> > > +
> > > #include "pan_context.h"
> > > #include "util/hash_table.h"
> > > #include "util/ralloc.h"
> > > @@ -124,8 +126,13 @@ panfrost_get_job_for_fbo(struct panfrost_context
> > > *ctx)
> > >
> > > /* If we already began rendering, use that */
> > >
> > > -if (ctx->job)
> > > +if (ctx->job) {
> > > +assert(ctx->job->key.zsbuf ==
> > > ctx->pipe_framebuffer.zsbuf &&
> > > + !memcmp(ctx->job->key.cbufs,
> > > + ctx->pipe_framebuffer.cbufs,
> > > + sizeof(ctx->job->key.cbufs)));
> > > return ctx->job;
> > > +}
> > >
> > > /* If not, look up the job */
> > >
> > > @@ -133,6 +140,10 @@ panfrost_get_job_for_fbo(struct panfrost_context
> > > *ctx)
> > > struct pipe_surface *zsbuf = ctx->pipe_framebuffer.zsbuf;
> > > struct panfrost_job *job = panfrost_get_job(ctx, cbufs, zsbuf);
> > >
> > > +/* Set this job as the current FBO job. Will be reset when
> > > updating the
> > > + * FB state and when submitting or releasing a job.
> > > + */
> > > +ctx->job = job;
> > > return job;
> > > }
> > >
> > > @@ -181,6 +192,12 @@ panfrost_job_submit(struct panfrost_context *ctx,
> > > struct panfrost_job *job)
> > >
> > > if (ret)
> > > fprintf(stderr, "panfrost_job_submit failed: %d\n", ret);
> > > +
> > > +/* The job has been submitted, let's invalidate the current FBO
> > > job
> > > + * cache.
> > > + */
> > > +assert(!ctx->job || job == ctx->job);
> > > +ctx->job = NULL;
> > > }
> > >
> > > void
> >
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 1/9] panfrost: Make ctx->job useful
@Boris I don't see why this patch particularly needs to be backported to
19.1?
On Thu, Aug 08, 2019 at 11:46:43AM +0200, Juan A. Suarez Romero wrote:
> On Fri, 2019-08-02 at 19:18 +0200, Boris Brezillon wrote:
> > ctx->job is supposed to serve as a cache to avoid an hash table lookup
> > everytime we access the job attached to the currently bound FB, except
> > it was never assigned to anything but NULL.
> >
> > Fix that by adding the missing assignment in panfrost_get_job_for_fbo().
> > Also add a missing NULL assignment in the ->set_framebuffer_state()
> > path.
> >
> > While at it, add extra assert()s to make sure ctx->job is consistent.
> >
> > Fixes: 59c9623d0a75 ("panfrost: Import job data structures from v3d")
> > Signed-off-by: Boris Brezillon
> > Changes in v2:
> > * New patch (suggested by Alyssa)
>
>
> This patch is a candidate for 19.1, but unfortunately it does not apply
> cleanly.
> It seems it depends on a bunch of other commits not in 19.1
>
> At this point, I will ignore/reject this patch for 19.1, unless you can
> provide
> me a specific version for 19.1 branch (use staging/19.1 as reference).
>
> Thanks.
>
> J.A.
>
> > ---
> > src/gallium/drivers/panfrost/pan_context.c | 5 +
> > src/gallium/drivers/panfrost/pan_job.c | 19 ++-
> > 2 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/gallium/drivers/panfrost/pan_context.c
> > b/src/gallium/drivers/panfrost/pan_context.c
> > index 014f8f6a9d07..ba2df2ce66ae 100644
> > --- a/src/gallium/drivers/panfrost/pan_context.c
> > +++ b/src/gallium/drivers/panfrost/pan_context.c
> > @@ -2372,6 +2372,11 @@ panfrost_set_framebuffer_state(struct pipe_context
> > *pctx,
> > panfrost_flush(pctx, NULL, PIPE_FLUSH_END_OF_FRAME);
> > }
> >
> > +/* Invalidate the FBO job cache since we've just been assigned a
> > new
> > + * FB state.
> > + */
> > +ctx->job = NULL;
> > +
> > util_copy_framebuffer_state(&ctx->pipe_framebuffer, fb);
> >
> > /* Given that we're rendering, we'd love to have compression */
> > diff --git a/src/gallium/drivers/panfrost/pan_job.c
> > b/src/gallium/drivers/panfrost/pan_job.c
> > index 6339b39d29a0..cc81d475165e 100644
> > --- a/src/gallium/drivers/panfrost/pan_job.c
> > +++ b/src/gallium/drivers/panfrost/pan_job.c
> > @@ -23,6 +23,8 @@
> > *
> > */
> >
> > +#include
> > +
> > #include "pan_context.h"
> > #include "util/hash_table.h"
> > #include "util/ralloc.h"
> > @@ -124,8 +126,13 @@ panfrost_get_job_for_fbo(struct panfrost_context *ctx)
> >
> > /* If we already began rendering, use that */
> >
> > -if (ctx->job)
> > +if (ctx->job) {
> > +assert(ctx->job->key.zsbuf == ctx->pipe_framebuffer.zsbuf
> > &&
> > + !memcmp(ctx->job->key.cbufs,
> > + ctx->pipe_framebuffer.cbufs,
> > + sizeof(ctx->job->key.cbufs)));
> > return ctx->job;
> > +}
> >
> > /* If not, look up the job */
> >
> > @@ -133,6 +140,10 @@ panfrost_get_job_for_fbo(struct panfrost_context *ctx)
> > struct pipe_surface *zsbuf = ctx->pipe_framebuffer.zsbuf;
> > struct panfrost_job *job = panfrost_get_job(ctx, cbufs, zsbuf);
> >
> > +/* Set this job as the current FBO job. Will be reset when
> > updating the
> > + * FB state and when submitting or releasing a job.
> > + */
> > +ctx->job = job;
> > return job;
> > }
> >
> > @@ -181,6 +192,12 @@ panfrost_job_submit(struct panfrost_context *ctx,
> > struct panfrost_job *job)
> >
> > if (ret)
> > fprintf(stderr, "panfrost_job_submit failed: %d\n", ret);
> > +
> > +/* The job has been submitted, let's invalidate the current FBO job
> > + * cache.
> > +*/
> > +assert(!ctx->job || job == ctx->job);
> > +ctx->job = NULL;
> > }
> >
> > void
>
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 1/9] panfrost: Make ctx->job useful
On Fri, 2019-08-02 at 19:18 +0200, Boris Brezillon wrote:
> ctx->job is supposed to serve as a cache to avoid an hash table lookup
> everytime we access the job attached to the currently bound FB, except
> it was never assigned to anything but NULL.
>
> Fix that by adding the missing assignment in panfrost_get_job_for_fbo().
> Also add a missing NULL assignment in the ->set_framebuffer_state()
> path.
>
> While at it, add extra assert()s to make sure ctx->job is consistent.
>
> Fixes: 59c9623d0a75 ("panfrost: Import job data structures from v3d")
> Signed-off-by: Boris Brezillon
> Changes in v2:
> * New patch (suggested by Alyssa)
This patch is a candidate for 19.1, but unfortunately it does not apply cleanly.
It seems it depends on a bunch of other commits not in 19.1
At this point, I will ignore/reject this patch for 19.1, unless you can provide
me a specific version for 19.1 branch (use staging/19.1 as reference).
Thanks.
J.A.
> ---
> src/gallium/drivers/panfrost/pan_context.c | 5 +
> src/gallium/drivers/panfrost/pan_job.c | 19 ++-
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/panfrost/pan_context.c
> b/src/gallium/drivers/panfrost/pan_context.c
> index 014f8f6a9d07..ba2df2ce66ae 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -2372,6 +2372,11 @@ panfrost_set_framebuffer_state(struct pipe_context
> *pctx,
> panfrost_flush(pctx, NULL, PIPE_FLUSH_END_OF_FRAME);
> }
>
> +/* Invalidate the FBO job cache since we've just been assigned a new
> + * FB state.
> + */
> +ctx->job = NULL;
> +
> util_copy_framebuffer_state(&ctx->pipe_framebuffer, fb);
>
> /* Given that we're rendering, we'd love to have compression */
> diff --git a/src/gallium/drivers/panfrost/pan_job.c
> b/src/gallium/drivers/panfrost/pan_job.c
> index 6339b39d29a0..cc81d475165e 100644
> --- a/src/gallium/drivers/panfrost/pan_job.c
> +++ b/src/gallium/drivers/panfrost/pan_job.c
> @@ -23,6 +23,8 @@
> *
> */
>
> +#include
> +
> #include "pan_context.h"
> #include "util/hash_table.h"
> #include "util/ralloc.h"
> @@ -124,8 +126,13 @@ panfrost_get_job_for_fbo(struct panfrost_context *ctx)
>
> /* If we already began rendering, use that */
>
> -if (ctx->job)
> +if (ctx->job) {
> +assert(ctx->job->key.zsbuf == ctx->pipe_framebuffer.zsbuf &&
> + !memcmp(ctx->job->key.cbufs,
> + ctx->pipe_framebuffer.cbufs,
> + sizeof(ctx->job->key.cbufs)));
> return ctx->job;
> +}
>
> /* If not, look up the job */
>
> @@ -133,6 +140,10 @@ panfrost_get_job_for_fbo(struct panfrost_context *ctx)
> struct pipe_surface *zsbuf = ctx->pipe_framebuffer.zsbuf;
> struct panfrost_job *job = panfrost_get_job(ctx, cbufs, zsbuf);
>
> +/* Set this job as the current FBO job. Will be reset when updating
> the
> + * FB state and when submitting or releasing a job.
> + */
> +ctx->job = job;
> return job;
> }
>
> @@ -181,6 +192,12 @@ panfrost_job_submit(struct panfrost_context *ctx, struct
> panfrost_job *job)
>
> if (ret)
> fprintf(stderr, "panfrost_job_submit failed: %d\n", ret);
> +
> +/* The job has been submitted, let's invalidate the current FBO job
> + * cache.
> + */
> +assert(!ctx->job || job == ctx->job);
> +ctx->job = NULL;
> }
>
> void
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 1/9] panfrost: Make ctx->job useful
R-b! Nice! :) signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 1/9] panfrost: Make ctx->job useful
ctx->job is supposed to serve as a cache to avoid an hash table lookup
everytime we access the job attached to the currently bound FB, except
it was never assigned to anything but NULL.
Fix that by adding the missing assignment in panfrost_get_job_for_fbo().
Also add a missing NULL assignment in the ->set_framebuffer_state()
path.
While at it, add extra assert()s to make sure ctx->job is consistent.
Fixes: 59c9623d0a75 ("panfrost: Import job data structures from v3d")
Signed-off-by: Boris Brezillon
Changes in v2:
* New patch (suggested by Alyssa)
---
src/gallium/drivers/panfrost/pan_context.c | 5 +
src/gallium/drivers/panfrost/pan_job.c | 19 ++-
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/gallium/drivers/panfrost/pan_context.c
b/src/gallium/drivers/panfrost/pan_context.c
index 014f8f6a9d07..ba2df2ce66ae 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -2372,6 +2372,11 @@ panfrost_set_framebuffer_state(struct pipe_context *pctx,
panfrost_flush(pctx, NULL, PIPE_FLUSH_END_OF_FRAME);
}
+/* Invalidate the FBO job cache since we've just been assigned a new
+ * FB state.
+ */
+ctx->job = NULL;
+
util_copy_framebuffer_state(&ctx->pipe_framebuffer, fb);
/* Given that we're rendering, we'd love to have compression */
diff --git a/src/gallium/drivers/panfrost/pan_job.c
b/src/gallium/drivers/panfrost/pan_job.c
index 6339b39d29a0..cc81d475165e 100644
--- a/src/gallium/drivers/panfrost/pan_job.c
+++ b/src/gallium/drivers/panfrost/pan_job.c
@@ -23,6 +23,8 @@
*
*/
+#include
+
#include "pan_context.h"
#include "util/hash_table.h"
#include "util/ralloc.h"
@@ -124,8 +126,13 @@ panfrost_get_job_for_fbo(struct panfrost_context *ctx)
/* If we already began rendering, use that */
-if (ctx->job)
+if (ctx->job) {
+assert(ctx->job->key.zsbuf == ctx->pipe_framebuffer.zsbuf &&
+ !memcmp(ctx->job->key.cbufs,
+ ctx->pipe_framebuffer.cbufs,
+ sizeof(ctx->job->key.cbufs)));
return ctx->job;
+}
/* If not, look up the job */
@@ -133,6 +140,10 @@ panfrost_get_job_for_fbo(struct panfrost_context *ctx)
struct pipe_surface *zsbuf = ctx->pipe_framebuffer.zsbuf;
struct panfrost_job *job = panfrost_get_job(ctx, cbufs, zsbuf);
+/* Set this job as the current FBO job. Will be reset when updating the
+ * FB state and when submitting or releasing a job.
+ */
+ctx->job = job;
return job;
}
@@ -181,6 +192,12 @@ panfrost_job_submit(struct panfrost_context *ctx, struct
panfrost_job *job)
if (ret)
fprintf(stderr, "panfrost_job_submit failed: %d\n", ret);
+
+/* The job has been submitted, let's invalidate the current FBO job
+ * cache.
+*/
+assert(!ctx->job || job == ctx->job);
+ctx->job = NULL;
}
void
--
2.21.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
