Re: [Mesa-dev] [PATCH 2/2] i965: Context aware user space EU control through application
> -Original Message-
> From: Landwerlin, Lionel G
> Sent: Tuesday, July 24, 2018 10:24 PM
> To: Marathe, Yogesh ; Chris Wilson wilson.co.uk>; Muthukumar, Aravindan ;
> mesa-dev@lists.freedesktop.org
> Cc: Diwakar, Praveen
> Subject: Re: [Mesa-dev] [PATCH 2/2] i965: Context aware user space EU control
> through application
>
> On 24/07/18 17:41, Marathe, Yogesh wrote:
> > Lionel, Chris,
> >
> >> -Original Message-
> >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> >> Behalf Of Lionel Landwerlin
> >> Sent: Friday, July 20, 2018 3:31 PM
> >> To: Chris Wilson ; Muthukumar, Aravindan
> >> ; mesa-dev@lists.freedesktop.org
> >> Subject: Re: [Mesa-dev] [PATCH 2/2] i965: Context aware user space EU
> >> control through application
> >>
> >> On 20/07/18 09:47, Chris Wilson wrote:
> >>> Quoting aravindan.muthuku...@intel.com (2018-07-20 09:32:57)
> >>>> From: "Muthukumar, Aravindan"
> >>>>
> >>>>The Patch here is to give control to user/ application to really
> >>>>decide what's the max GPU load it would put. If that can be
> >>>>known in advance, rpcs can be programmed accordingly.
> >>>>This solution has changes across i915,
> >>>>drm and mesa (not limited only to kernel).
> >>>>
> >>>>Here, we pass gpu_load_type = {high, medium, low} from application
> >>>>while context is created. Default here is 'High' and applications
> >>>>roughly know if they are going to eat up entire GPU. The typical
> >>>>usecase of 'Low' is idle screen or minor mouse movements. Users can
> >>>>read meaning of high/medium/low for their platform & then program
> >>>>contexts accordingly. Here gpu_load_type directly translates to
> >>>>number of shader cores/EUs a particular GPU has.
> >>>>
> >>>>Signed-off-by: Aravindan Muthukumar
>
> >>>>Signed-off-by: Kedar J Karanje
> >>>>Signed-off-by: Praveen Diwakar
> >>>>Signed-off-by: Yogesh Marathe
> >>>> +/* Dynamic Eu control */
> >>>> +struct drm_i915_load_type {
> >>>> + __u32 ctx_id;
> >>>> + __u32 load_type;
> >>>> +};
> >>>> +
> >>>> +/* DYNAMIC EU CONTROL */
> >>>> +int
> >>>> +brw_hw_context_load_type(struct brw_bufmgr *bufmgr,
> >>>> +uint32_t ctx_id,
> >>>> +int load_type) {
> >>>> + struct drm_i915_load_type type = {
> >>>> + .ctx_id = ctx_id,
> >>>> + .load_type = load_type,
> >>>> + };
> >>>> + int err;
> >>>> +
> >>>> + err = 0;
> >>>> + if(drmIoctl(bufmgr->fd, DRM_IOCTL_I915_LOAD_TYPE, &type))
> >>>> + err = -errno;
> >>> This went through 4 people and none noticed that there already
> >>> exists a means to set per-context parameters. And it's even used
> >>> right next to this function.
> >>>
> >>> The word hint needs to be firmly embedded around here.
> >>> -Chris
> >>> __
> >> Yep,
> >>
> >> Looks like you want to get involved in this discussion :
> >> https://patchwork.freedesktop.org/series/42285/
> > I understand this is exposing per context eu config through debugfs.
> > That mostly (if not fully) matches the kernel part of what we wanted
> > to achieve. We have additional code in kernel where we categorize
> > based on load type and fix a config per platform. For sure the kernel
> > parts can be merged but the proposal is different here and its specific to
> adding this capability through mesa.
> >
> > Here we are enabling applications to decide load while creating the
> > context and making it simple for application programmers by
> > abstracting it. Also in these kernel patches, its seems to be
> > exposing the parameters to user space, are we discussing its user
> > space counterpart in mesa or in some other component? If not, I feel
> > this is bit different. Can it be a mesa extension? Then any app / process
> > can do
> this without having privilege (root).
>
> Yes, I was just pointing out that you might want to reuse existing patches for
> i915 so that we don't end up with 2 similar interfaces there.
> Obviously your extension would still be needed.
>
Thanks, that’s clear now.
> -
> Lionel
>
> >
> >> -
> >> Lionel
> >> ___
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: Context aware user space EU control through application
Lionel, Chris,
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Lionel Landwerlin
> Sent: Friday, July 20, 2018 3:31 PM
> To: Chris Wilson ; Muthukumar, Aravindan
> ; mesa-dev@lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH 2/2] i965: Context aware user space EU control
> through application
>
> On 20/07/18 09:47, Chris Wilson wrote:
> > Quoting aravindan.muthuku...@intel.com (2018-07-20 09:32:57)
> >> From: "Muthukumar, Aravindan"
> >>
> >> The Patch here is to give control to user/ application to really
> >> decide what's the max GPU load it would put. If that can be
> >> known in advance, rpcs can be programmed accordingly.
> >> This solution has changes across i915,
> >> drm and mesa (not limited only to kernel).
> >>
> >> Here, we pass gpu_load_type = {high, medium, low} from application
> >> while context is created. Default here is 'High' and applications
> >> roughly know if they are going to eat up entire GPU. The typical
> >> usecase of 'Low' is idle screen or minor mouse movements. Users can
> >> read meaning of high/medium/low for their platform & then program
> >> contexts accordingly. Here gpu_load_type directly translates to
> >> number of shader cores/EUs a particular GPU has.
> >>
> >> Signed-off-by: Aravindan Muthukumar
> >> Signed-off-by: Kedar J Karanje
> >> Signed-off-by: Praveen Diwakar
> >> Signed-off-by: Yogesh Marathe
> >> +/* Dynamic Eu control */
> >> +struct drm_i915_load_type {
> >> + __u32 ctx_id;
> >> + __u32 load_type;
> >> +};
> >> +
> >> +/* DYNAMIC EU CONTROL */
> >> +int
> >> +brw_hw_context_load_type(struct brw_bufmgr *bufmgr,
> >> +uint32_t ctx_id,
> >> +int load_type) {
> >> + struct drm_i915_load_type type = {
> >> + .ctx_id = ctx_id,
> >> + .load_type = load_type,
> >> + };
> >> + int err;
> >> +
> >> + err = 0;
> >> + if(drmIoctl(bufmgr->fd, DRM_IOCTL_I915_LOAD_TYPE, &type))
> >> + err = -errno;
> > This went through 4 people and none noticed that there already exists
> > a means to set per-context parameters. And it's even used right next
> > to this function.
> >
> > The word hint needs to be firmly embedded around here.
> > -Chris
> > __
>
> Yep,
>
> Looks like you want to get involved in this discussion :
> https://patchwork.freedesktop.org/series/42285/
I understand this is exposing per context eu config through debugfs. That mostly
(if not fully) matches the kernel part of what we wanted to achieve. We have
additional code in kernel where we categorize based on load type and fix
a config per platform. For sure the kernel parts can be merged but the proposal
is different here and its specific to adding this capability through mesa.
Here we are enabling applications to decide load while creating the context and
making it simple for application programmers by abstracting it. Also in these
kernel
patches, its seems to be exposing the parameters to user space, are we
discussing
its user space counterpart in mesa or in some other component? If not, I feel
this
is bit different. Can it be a mesa extension? Then any app / process can do this
without having privilege (root).
>
> -
> Lionel
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Proposing an extension to mesa
Hi, We want to propose an extension to mesa (GL & EGL), is there a separate way to do this or that's though RFC and patches on this list here? We have implemented it for i965 but we believe it can be used for other drivers as well. Please let us know. Regards, Yogesh. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/x11: Move dri2_format_for_depth prototype.
> -Original Message-
> From: Engestrom, Eric
> Sent: Tuesday, May 29, 2018 7:48 PM
> To: Marathe, Yogesh
> Cc: Vinson Lee ; Daniel Stone
> ; Emil Velikov ; Eric
> Engestrom ; ML mesa-dev d...@lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [PATCH] egl/x11: Move dri2_format_for_depth
> prototype.
>
> On Tuesday, 2018-05-29 05:21:02 +0100, Marathe, Yogesh wrote:
> > Hi Vinson, Eric,
> >
> > > -Original Message-
> > > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> > > Behalf Of Vinson Lee
> > > Sent: Sunday, May 27, 2018 1:34 PM
> > > To: Engestrom, Eric
> > > Cc: Daniel Stone ; Emil Velikov
> > > ; Eric Engestrom ;
> > > Marathe, Yogesh ; ML mesa-dev > > d...@lists.freedesktop.org>
> > > Subject: Re: [Mesa-dev] [PATCH] egl/x11: Move dri2_format_for_depth
> > > prototype.
> > >
> > > On Fri, May 25, 2018 at 9:42 AM, Eric Engestrom
> > >
> > > wrote:
> > > > On Friday, 2018-05-25 16:06:26 +0100, Eric Engestrom wrote:
> > > >> On Friday, 2018-05-25 06:52:25 +, Vinson Lee wrote:
> > > >> > Fix build error without DRI3.
> > > >>
> > > >> D'uh!
> > > >> I forgot building dri3 was optional, sorry :/
> > > >>
> > > >> Reviewed-by: Eric Engestrom
> > > >
> > > > Actually, wait no, this doesn't look right, the function should be
> > > > named something else if it's exposed to everyone, since it's quite
> > > > specific to x11's case, or it should not be exposed to everyone.
> > > >
> > > > I feel like the best thing to do here is to just copy the
> > > > prototype to
> > > > platform_x11.c:
> > > >
> > > > ---8<---
> > > > diff --git a/src/egl/drivers/dri2/platform_x11.c
> > > > b/src/egl/drivers/dri2/platform_x11.c
> > > > index b2a3000b252ec0ddb12f..ea9b0cc6d6fd04804d2a 100644
> > > > --- a/src/egl/drivers/dri2/platform_x11.c
> > > > +++ b/src/egl/drivers/dri2/platform_x11.c
> > > > @@ -55,6 +55,9 @@ static EGLBoolean
> > > > dri2_x11_swap_interval(_EGLDriver *drv, _EGLDisplay *disp,
> > > > _EGLSurface
> > > *surf,
> > > > EGLint interval);
> > > >
> > > > +uint32_t
> > > > +dri2_format_for_depth(uint32_t depth); u
> > > > static void
> > > > swrastCreateDrawable(struct dri2_egl_display * dri2_dpy,
> > > > struct dri2_egl_surface * dri2_surf)
> > > > --->8---
> > > >
> > >
> > > This also fixes the build error.
> > >
> > > Tested-by: Vinson Lee
> > >
> >
> > Did we also check dri3 build after this?
>
> I checked with my usual "everything is enabled" build as well as the default
> meson configuration.
>
> > I see platform_x11_dri3.c using it and we've removed it from
> > platform_x11_dri3.h, dri2_format_for_depth() prototype with extern in
> > platform_x11_dri3.c shall help?
>
> I'm not sure I follow, but I think you're talking about adding my inline
> platform_x11.c patch on top of vlee's original patch, which is not what I
> suggested; my suggestion was to replace his patch with mine.
> Does that answer your question?
>
Yes, I'm good. It means for dri3 prototype still resides in platform_x11_dri3.h
Sorry, I missed words _just copy_ in your mail.
Reviewed-by: Yogesh Marathe
> >
> > If not extern I'm inclining towards better name for the function in
> > egl_dri2.h
> >
> > > >>
> > > >> >
> > > >> > CC drivers/dri2/platform_x11.lo
> > > >> > drivers/dri2/platform_x11.c:1010:1: error: no previous
> > > >> > prototype for function 'dri2_format_for_depth'
> > > >> > [-Werror,-Wmissing-prototypes] dri2_format_for_depth(uint32_t
> > > >> > depth) ^
> > > >> >
> > > >> > Fixes: 473af0b541b2 ("egl/x11: deduplicate depth-to-format
> > > >> > logic")
> > > >> > Signed-off-by: Vinson Lee
> > > >> > ---
> > > >> > src/egl/drivers/dri2/egl_dri2.h | 3 +++
> > > >> > src/egl/drivers/dri2/platform_x11_dri3.h | 3 ---
> > > >> > 2 files changed, 3 insertions(+), 3 deletion
Re: [Mesa-dev] [PATCH] egl/x11: Move dri2_format_for_depth prototype.
Hi Vinson, Eric,
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Vinson Lee
> Sent: Sunday, May 27, 2018 1:34 PM
> To: Engestrom, Eric
> Cc: Daniel Stone ; Emil Velikov
> ; Eric Engestrom ; Marathe,
> Yogesh ; ML mesa-dev d...@lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [PATCH] egl/x11: Move dri2_format_for_depth
> prototype.
>
> On Fri, May 25, 2018 at 9:42 AM, Eric Engestrom
> wrote:
> > On Friday, 2018-05-25 16:06:26 +0100, Eric Engestrom wrote:
> >> On Friday, 2018-05-25 06:52:25 +, Vinson Lee wrote:
> >> > Fix build error without DRI3.
> >>
> >> D'uh!
> >> I forgot building dri3 was optional, sorry :/
> >>
> >> Reviewed-by: Eric Engestrom
> >
> > Actually, wait no, this doesn't look right, the function should be
> > named something else if it's exposed to everyone, since it's quite
> > specific to x11's case, or it should not be exposed to everyone.
> >
> > I feel like the best thing to do here is to just copy the prototype to
> > platform_x11.c:
> >
> > ---8<---
> > diff --git a/src/egl/drivers/dri2/platform_x11.c
> > b/src/egl/drivers/dri2/platform_x11.c
> > index b2a3000b252ec0ddb12f..ea9b0cc6d6fd04804d2a 100644
> > --- a/src/egl/drivers/dri2/platform_x11.c
> > +++ b/src/egl/drivers/dri2/platform_x11.c
> > @@ -55,6 +55,9 @@ static EGLBoolean
> > dri2_x11_swap_interval(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface
> *surf,
> > EGLint interval);
> >
> > +uint32_t
> > +dri2_format_for_depth(uint32_t depth); u
> > static void
> > swrastCreateDrawable(struct dri2_egl_display * dri2_dpy,
> > struct dri2_egl_surface * dri2_surf)
> > --->8---
> >
>
> This also fixes the build error.
>
> Tested-by: Vinson Lee
>
Did we also check dri3 build after this? I see platform_x11_dri3.c using it
and we've removed it from platform_x11_dri3.h, dri2_format_for_depth()
prototype with extern in platform_x11_dri3.c shall help?
If not extern I'm inclining towards better name for the function in egl_dri2.h
> >>
> >> >
> >> > CC drivers/dri2/platform_x11.lo
> >> > drivers/dri2/platform_x11.c:1010:1: error: no previous prototype
> >> > for function 'dri2_format_for_depth' [-Werror,-Wmissing-prototypes]
> >> > dri2_format_for_depth(uint32_t depth) ^
> >> >
> >> > Fixes: 473af0b541b2 ("egl/x11: deduplicate depth-to-format logic")
> >> > Signed-off-by: Vinson Lee
> >> > ---
> >> > src/egl/drivers/dri2/egl_dri2.h | 3 +++
> >> > src/egl/drivers/dri2/platform_x11_dri3.h | 3 ---
> >> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/src/egl/drivers/dri2/egl_dri2.h
> >> > b/src/egl/drivers/dri2/egl_dri2.h index adabc527f85b..b91a899e476c
> >> > 100644
> >> > --- a/src/egl/drivers/dri2/egl_dri2.h
> >> > +++ b/src/egl/drivers/dri2/egl_dri2.h
> >> > @@ -523,4 +523,7 @@ dri2_init_surface(_EGLSurface *surf,
> >> > _EGLDisplay *dpy, EGLint type, void dri2_fini_surface(_EGLSurface
> >> > *surf);
> >> >
> >> > +uint32_t
> >> > +dri2_format_for_depth(uint32_t depth);
> >> > +
> >> > #endif /* EGL_DRI2_INCLUDED */
> >> > diff --git a/src/egl/drivers/dri2/platform_x11_dri3.h
> >> > b/src/egl/drivers/dri2/platform_x11_dri3.h
> >> > index e6fd01366978..96e7ee972d9f 100644
> >> > --- a/src/egl/drivers/dri2/platform_x11_dri3.h
> >> > +++ b/src/egl/drivers/dri2/platform_x11_dri3.h
> >> > @@ -38,7 +38,4 @@ extern struct dri2_egl_display_vtbl
> >> > dri3_x11_display_vtbl; EGLBoolean dri3_x11_connect(struct
> >> > dri2_egl_display *dri2_dpy);
> >> >
> >> > -uint32_t
> >> > -dri2_format_for_depth(uint32_t depth);
> >> > -
> >> > #endif
> >> > --
> >> > 2.17.0
> >> >
> >> > ___
> >> > mesa-dev mailing list
> >> > mesa-dev@lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [AppVeyor] mesa master #6216 failed
Thanks Ilia. >-Original Message- >From: ibmir...@gmail.com [mailto:ibmir...@gmail.com] On Behalf Of Ilia Mirkin >Sent: Tuesday, November 21, 2017 10:11 AM >To: Marathe, Yogesh >Cc: mesa-dev@lists.freedesktop.org; Muthukumar, Aravindan > >Subject: Re: [Mesa-dev] [AppVeyor] mesa master #6216 failed > >Build started >git clone -q --depth=100 --branch=master >git://anongit.freedesktop.org/mesa/mesa C:\projects\mesa >warning: Could not find remote branch master to clone. >fatal: Remote branch master not found in upstream origin Command exited with >code 128 > >Not your commit's fault. > >On Mon, Nov 20, 2017 at 11:33 PM, Marathe, Yogesh > wrote: >> Don't know if this is a concern. The patch was built & tested with intel >> mesa CI >and locally. Does this need attention or can be ignored? >> Please advise. >> >> -Yogesh. >> >>>From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On >>>Behalf Of AppVeyor >>>Sent: Tuesday, November 21, 2017 4:23 AM >>>To: mesa-dev@lists.freedesktop.org >>>Subject: [Mesa-dev] [AppVeyor] mesa master #6216 failed >>> >>>Build mesa 6216 failed >>>Commit 971b3c019b by Aravindan Muthukumar on 11/9/2017 5:45 AM: >>>i965: Optimize bucket index calculation\n\nReducing Bucket index >>>calculation to O(1).\n\nThis algorithm calculates the index using >>>matrix method. Assuming\nPAGE_SIZE is 4096, matrix arrangement is as >>>below:\n\n 1*4096 2*4096 3*4096 4*4096\n 5*4096 6*4096 7*4096 8*4096\n >>>10*4096 12*4096 14*4096 16*4096\n 20*4096 24*4096 28*4096 32*4096\n >... >>>... ... ...\n ... ... ... ...\n ... ... ... max_cache_size\n\nFrom >>>this matrix its clearly seen that every row follows the below way:\n\n ... >>>... ... n\n n+(1/4)n n+(1/2)n n+(3/4)n 2n\n\nRow is calculated as >>>log2(size/PAGE_SIZE) Column is calculated as\nconverting the >>>difference between the elements to fit into power size of\ntwo and >>>indexing it.\n\nFinal Index is (row*4)+(col-1)\n\nTested with Intel >>>Mesa CI.\n\nImproves performance of 3DMark on BXT by 0.705966% +/- >>>0.229767% >>>(n=20)\n\nv4: Review comments on style and code comments implemented >>>(Ian).\nv3: Review comments implemented (Ian).\nv2: Review comments >>>implemented (Jason).\n\nSigned-off-by: Aravindan Muthukumar >>>\nSigned-off-by: Kedar Karanje >>>\nReviewed-by: Yogesh Marathe >>>\nSigned-off-by: Ian Romanick >>> Configure your notification preferences >>> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [AppVeyor] mesa master #6216 failed
Don't know if this is a concern. The patch was built & tested with intel mesa CI and locally. Does this need attention or can be ignored? Please advise. -Yogesh. >From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On >Behalf Of AppVeyor >Sent: Tuesday, November 21, 2017 4:23 AM >To: mesa-dev@lists.freedesktop.org >Subject: [Mesa-dev] [AppVeyor] mesa master #6216 failed > >Build mesa 6216 failed >Commit 971b3c019b by Aravindan Muthukumar on 11/9/2017 5:45 AM: >i965: Optimize bucket index calculation\n\nReducing Bucket index >calculation to O(1).\n\nThis algorithm calculates the index using >matrix method. Assuming\nPAGE_SIZE is 4096, matrix arrangement is as >below:\n\n 1*4096 2*4096 3*4096 4*4096\n 5*4096 6*4096 7*4096 8*4096\n >10*4096 12*4096 14*4096 16*4096\n 20*4096 24*4096 28*4096 32*4096\n ... >... ... ...\n ... ... ... ...\n ... ... ... max_cache_size\n\nFrom this >matrix its clearly seen that every row follows the below way:\n\n ... >... ... n\n n+(1/4)n n+(1/2)n n+(3/4)n 2n\n\nRow is calculated as >log2(size/PAGE_SIZE) Column is calculated as\nconverting the difference >between the elements to fit into power size of\ntwo and indexing >it.\n\nFinal Index is (row*4)+(col-1)\n\nTested with Intel Mesa >CI.\n\nImproves performance of 3DMark on BXT by 0.705966% +/- 0.229767% >(n=20)\n\nv4: Review comments on style and code comments implemented >(Ian).\nv3: Review comments implemented (Ian).\nv2: Review comments >implemented (Jason).\n\nSigned-off-by: Aravindan Muthukumar >\nSigned-off-by: Kedar Karanje >\nReviewed-by: Yogesh Marathe >\nSigned-off-by: Ian Romanick > Configure your notification preferences > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965 : optimized bucket index calculation
Ian, Rest all review comments noted, we'll get back, Thanks. >-Original Message- >From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of >Ian Romanick >Sent: Friday, October 20, 2017 9:51 AM >To: Muthukumar, Aravindan ; mesa- >> Average : 201.2 +/- 65.4836 (n=20) > >Is 201 the improvement or the absolute score? Do not quote absolute scores. > No, this is not an absolute score this is the difference patch made and the deviation reported in requested format. -Yogesh. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/3] mesa: use simple mtx in core mesa
Hi Timothy,
>-Original Message-
>From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of
>Timothy Arceri
>Sent: Monday, October 16, 2017 6:30 AM
>To: mesa-dev@lists.freedesktop.org
>Cc: Timothy Arceri
>Subject: [Mesa-dev] [PATCH v2 2/3] mesa: use simple mtx in core mesa
>
>Results from x11perf -copywinwin10 on Eric's SKL:
> 4.8% ± 0.905054% (n=40)
I tested this on my i7 linux pc. If I observe data generated by perf for driver
overhead,
I can see cpu cycles spent in libpthread so gone down by almost 6% but at the
same time
i965_dri shows its consuming more cycles (same 6% more). Not much variation in
resultant fps either. I believe it's just removing dependency on libpthread,
any comments?
Another point, with patch I still see pthread mutex lock / unlock being used,
so I think
those are still being used in more places along with simple mtx.
Either case
Tested-by: Yogesh Marathe
>
>Reviewed-by: Marek Olšák
>---
> src/gallium/state_trackers/dri/dri2.c | 22 +++---
> src/mesa/main/bufferobj.c | 14 +++---
> src/mesa/main/debug_output.c | 16
> src/mesa/main/errors.c| 4 ++--
> src/mesa/main/fbobject.c | 14 +++---
> src/mesa/main/framebuffer.c | 14 +++---
> src/mesa/main/mtypes.h| 15 ---
> src/mesa/main/renderbuffer.c | 12 ++--
> src/mesa/main/robustness.c| 4 ++--
> src/mesa/main/samplerobj.c| 12 ++--
> src/mesa/main/shared.c| 12 ++--
> src/mesa/main/syncobj.c | 14 +++---
> src/mesa/main/texobj.c| 16
> src/mesa/vbo/vbo_minmax_index.c | 8
> 14 files changed, 89 insertions(+), 88 deletions(-)
>
>diff --git a/src/gallium/state_trackers/dri/dri2.c
>b/src/gallium/state_trackers/dri/dri2.c
>index e0cd0e0bc7c..a70f37fe09a 100644
>--- a/src/gallium/state_trackers/dri/dri2.c
>+++ b/src/gallium/state_trackers/dri/dri2.c
>@@ -1696,7 +1696,7 @@ dri2_interop_export_object(__DRIcontext *_ctx,
> return MESA_GLINTEROP_INVALID_MIP_LEVEL;
>
>/* Validate the OpenGL object and get pipe_resource. */
>- mtx_lock(&ctx->Shared->Mutex);
>+ simple_mtx_lock(&ctx->Shared->Mutex);
>
>if (target == GL_ARRAY_BUFFER) {
> /* Buffer objects.
>@@ -1712,14 +1712,14 @@ dri2_interop_export_object(__DRIcontext *_ctx,
>* the size of the buffer is 0."
>*/
> if (!buf || buf->Size == 0) {
>- mtx_unlock(&ctx->Shared->Mutex);
>+ simple_mtx_unlock(&ctx->Shared->Mutex);
> return MESA_GLINTEROP_INVALID_OBJECT;
> }
>
> res = st_buffer_object(buf)->buffer;
> if (!res) {
> /* this shouldn't happen */
>- mtx_unlock(&ctx->Shared->Mutex);
>+ simple_mtx_unlock(&ctx->Shared->Mutex);
> return MESA_GLINTEROP_INVALID_OBJECT;
> }
>
>@@ -1740,7 +1740,7 @@ dri2_interop_export_object(__DRIcontext *_ctx,
>*object or if the width or height of renderbuffer is zero."
>*/
> if (!rb || rb->Width == 0 || rb->Height == 0) {
>- mtx_unlock(&ctx->Shared->Mutex);
>+ simple_mtx_unlock(&ctx->Shared->Mutex);
> return MESA_GLINTEROP_INVALID_OBJECT;
> }
>
>@@ -1749,7 +1749,7 @@ dri2_interop_export_object(__DRIcontext *_ctx,
>*renderbuffer object."
>*/
> if (rb->NumSamples > 1) {
>- mtx_unlock(&ctx->Shared->Mutex);
>+ simple_mtx_unlock(&ctx->Shared->Mutex);
> return MESA_GLINTEROP_INVALID_OPERATION;
> }
>
>@@ -1759,7 +1759,7 @@ dri2_interop_export_object(__DRIcontext *_ctx,
>*/
> res = st_renderbuffer(rb)->texture;
> if (!res) {
>- mtx_unlock(&ctx->Shared->Mutex);
>+ simple_mtx_unlock(&ctx->Shared->Mutex);
> return MESA_GLINTEROP_OUT_OF_RESOURCES;
> }
>
>@@ -1789,7 +1789,7 @@ dri2_interop_export_object(__DRIcontext *_ctx,
> obj->Target != target ||
> !obj->_BaseComplete ||
> (in->miplevel > 0 && !obj->_MipmapComplete)) {
>- mtx_unlock(&ctx->Shared->Mutex);
>+ simple_mtx_unlock(&ctx->Shared->Mutex);
> return MESA_GLINTEROP_INVALID_OBJECT;
> }
>
>@@ -1802,19 +1802,19 @@ dri2_interop_export_object(__DRIcontext *_ctx,
>*specification and section 3.7.10 of the OpenGL ES 2.0."
>*/
> if (in->miplevel < obj->BaseLevel || in->miplevel > obj->_MaxLevel) {
>- mtx_unlock(&ctx->Shared->Mutex);
>+ simple_mtx_unlock(&ctx->Shared->Mutex);
> return MESA_GLINTEROP_INVALID_MIP_LEVEL;
> }
>
> if (!st_finalize_texture(ctx, st->pipe, obj, 0)) {
>- mtx_unlock(&ctx->Shared->Mutex);
>+ simple_mtx_unlock(&ctx->Shared->Mutex);
> return MESA_GLINTEROP_OUT_OF_RESOURCES;
> }
>
> res = st_get_texobj_resource(obj);
> if (!res) {
>
Re: [Mesa-dev] Testing out Kristian's fast mtx patch
3DMark I doubt, there is a 'driver overhead' sub-test under GFXBench which may show the difference here. Overall, I never saw these benchmarks spending higher CPU % in libpthread (mutexes), so I'm skeptical, it will affect anything in fps / total scores by large. Thank you for pointing this out anyways, I should still try this. -Yogesh. >-Original Message- >From: Palli, Tapani >Sent: Tuesday, October 10, 2017 2:37 PM >To: Timothy Arceri ; mesa-dev@lists.freedesktop.org >Cc: Marathe, Yogesh >Subject: Re: [Mesa-dev] Testing out Kristian's fast mtx patch > >It's possible that this series could help with CPU intensive 3DMark ... >FYI Yogesh. > > >On 10/10/2017 05:45 AM, Timothy Arceri wrote: >> After a recent discussion about this code from 2015 I was curious to >> give it a try. The outstanding review item was that we shouldn't be >> replacing the C11 mtx type/functions with our own, so I've renamed the >> fast path to simple_mtx* and added a couple of patches to make use of >> it. >> >> The idea is this fast mtx can be used in place of the full mtx >> implementation when its of type mtx_plain. >> >> I though if anywhere we might see a change in the drawoverhead piglit >> test but I didn't see any real change. >> >> Anyway since I've made the updates I thought I'd send it out. Maybe >> someone else might find some better results. Kristian reported a 10% >> increase in some internal Intel benchmarks, I wonder if thats still >> the case. >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965 : optimized bucket index calculation
+ all v1 reviewers. Does this look ok?
-Yogesh.
>-Original Message-
>From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of
>aravindan.muthuku...@intel.com
>Sent: Thursday, September 14, 2017 12:13 PM
>To: mesa-dev@lists.freedesktop.org
>Cc: Muthukumar, Aravindan ; J Karanje,
>Kedar
>Subject: [Mesa-dev] [PATCH v2] i965 : optimized bucket index calculation
>
>From: Aravindan Muthukumar
>
>Avoiding the loop which was running with O(n) complexity.
>Now the complexity has been reduced to O(1)
>
>Algorithm calculates the index using matrix method.
>Matrix arrangement is as below:
>Assuming PAGE_SIZE is 4096.
>
> 1*4096 2*40963*40964*4096
> 5*4096 6*40967*40968*4096
> ... ... ... ...
> ... ... ... ...
> ... ... ... max_cache_size
>
>From this matrix its cleary seen that every row follows the below way:
> ... ... ...n
> n+(1/4)n n+(1/2)n n+(3/4)n2n
>
>Row is calulated as log2(size/PAGE_SIZE) Column is calculated as converting the
>difference between the elements to fit into power size of two and indexing it.
>
>Final Index is (row*4)+(col-1)
>
>Tested with Intel Mesa CI.
>
>Improves performance of 3d Mark on Broxton.
>Analyzed using Compare Perf Analyser:
>Average : 201.2 +/- 65.4836 (n=20)
>Percentage : 0.705966% +/- 0.229767% (n=20)
>
>v2: Review comments regarding cosmetics and asserts implemented
>
>Signed-off-by: Aravindan Muthukumar
>Signed-off-by: Kedar Karanje
>Reviewed-by: Yogesh Marathe
>---
> src/mesa/drivers/dri/i965/brw_bufmgr.c | 46
>--
> 1 file changed, 39 insertions(+), 7 deletions(-)
>
>diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
>b/src/mesa/drivers/dri/i965/brw_bufmgr.c
>index 8017219..8013ccb 100644
>--- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
>+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
>@@ -87,6 +87,8 @@
>
> #define memclear(s) memset(&s, 0, sizeof(s))
>
>+#define PAGE_SIZE 4096
>+
> #define FILE_DEBUG_FLAG DEBUG_BUFMGR
>
> static inline int
>@@ -181,19 +183,45 @@ bo_tile_pitch(struct brw_bufmgr *bufmgr, uint32_t
>pitch, uint32_t tiling)
>return ALIGN(pitch, tile_width);
> }
>
>+static inline int
>+ilog2_round_up(int value)
>+{
>+ assert(value != 0);
>+ return 32 - __builtin_clz(value - 1); }
>+
>+/*
>+ * This function finds the correct bucket fit for the input size.
>+ * The function works with O(1) complexity when the requested size
>+ * was queried instead of iterating the size through all the buckets.
>+ */
> static struct bo_cache_bucket *
> bucket_for_size(struct brw_bufmgr *bufmgr, uint64_t size) {
>- int i;
>+ int index = -1;
>+ int row, col = 0;
>+ int pages, pages_log2;
>
>- for (i = 0; i < bufmgr->num_buckets; i++) {
>- struct bo_cache_bucket *bucket = &bufmgr->cache_bucket[i];
>- if (bucket->size >= size) {
>- return bucket;
>- }
>+ /* condition for size less than 4*4096 (16KB) page size */
>+ if(size <= 4 * PAGE_SIZE) {
>+ index = DIV_ROUND_UP(size, PAGE_SIZE) - 1;;
>+ } else {
>+ /* Number of pages of page size */
>+ pages = DIV_ROUND_UP(size, PAGE_SIZE);
>+ pages_log2 = ilog2_round_up(pages) - 1;
>+
>+ /* Finding the row and column of the matrix */
>+ row = pages_log2 - 1;
>+ col = DIV_ROUND_UP((pages - (1 << pages_log2)),
>+(1 << (pages_log2 - 2)));
>+
>+ /* Using the calculated row and column to index into the matrix */
>+ index = (row << 2) + (col - 1);
>}
>
>- return NULL;
>+ /* Checking the error condition */
>+ return (index >= 0 && index < bufmgr->num_buckets) ?
>+ (&bufmgr->cache_bucket[index]) : NULL;
> }
>
> int
>@@ -1239,6 +1267,10 @@ add_bucket(struct brw_bufmgr *bufmgr, int size)
>list_inithead(&bufmgr->cache_bucket[i].head);
>bufmgr->cache_bucket[i].size = size;
>bufmgr->num_buckets++;
>+
>+ assert(bucket_for_size(bufmgr, size) == &bufmgr->cache_bucket[i]);
>+ assert(bucket_for_size(bufmgr, size - 2048) == &bufmgr->cache_bucket[i]);
>+ assert(bucket_for_size(bufmgr, size + 1) !=
>+ &bufmgr->cache_bucket[i]);
> }
>
> static void
>--
>2.7.4
>
>___
>mesa-dev mailing list
>mesa-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/3] egl/android: Use per surface out fence
Hi Emil, as discussed, we are leaving this at 3/3 split you've created.
I've marked my earlier series "Not Applicable" in patchwork. Thanks.
-Yogesh.
>-Original Message-
>From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of
>Emil Velikov
>Sent: Friday, September 15, 2017 11:03 PM
>To: mesa-dev@lists.freedesktop.org
>Cc: emil.l.veli...@gmail.com
>Subject: [Mesa-dev] [PATCH 3/3] egl/android: Use per surface out fence
>
>From: Zhongmin Wu
>
>Use the plumbing introduced with previous patch, to interact with the Android
>framework.
>
>Namely: currently we use an invalid fd or -1 for our calls to
>ANativeWindow::{queue,cancel}Buffer.
>
>At the same time applications (like flatland) may rely on it being a valid
>one. Thus
>as they attempt to query the timestamp of the fence, they get unexpected
>results/behaviour.
>
>In the case of flatland - the benchmark hand inside getSignalTime().
>
>Make use of the out fence and pass the correct fd to Android.
>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101655
>Signed-off-by: Zhongmin Wu
>Signed-off-by: Yogesh Marathe
>Reviewed-by: Emil Velikov
>Reviewed-by: Tomasz Figa [Emil Velikov: split from larger
>patch]
>Signed-off-by: Emil Velikov
>---
> src/egl/drivers/dri2/platform_android.c | 28 +++-
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
>diff --git a/src/egl/drivers/dri2/platform_android.c
>b/src/egl/drivers/dri2/platform_android.c
>index 38c1122339f..d08a8b22b7d 100644
>--- a/src/egl/drivers/dri2/platform_android.c
>+++ b/src/egl/drivers/dri2/platform_android.c
>@@ -229,19 +229,18 @@ droid_window_enqueue_buffer(_EGLDisplay *disp,
>struct dri2_egl_surface *dri2_sur
> */
>mtx_unlock(&disp->Mutex);
>
>- /* Queue the buffer without a sync fence. This informs the ANativeWindow
>-* that it may access the buffer immediately.
>+ /* Queue the buffer with stored out fence fd. The ANativeWindow or buffer
>+* consumer may choose to wait for the fence to signal before accessing
>+* it. If fence fd value is -1, buffer can be accessed by consumer
>+* immediately. Consumer or application shouldn't rely on timestamp
>+* associated with fence if the fence fd is -1.
> *
>-* From ANativeWindow::dequeueBuffer:
>-*
>-*The fenceFd argument specifies a libsync fence file descriptor for
>-*a fence that must signal before the buffer can be accessed. If
>-*the buffer can be accessed immediately then a value of -1 should
>-*be used. The caller must not use the file descriptor after it
>-*is passed to queueBuffer, and the ANativeWindow implementation
>-*is responsible for closing it.
>+* Ownership of fd is transferred to consumer after queueBuffer and the
>+* consumer is responsible for closing it. Caller must not use the fd
>+* after passing it to queueBuffer.
> */
>- int fence_fd = -1;
>+ int fence_fd = dri2_surf->out_fence_fd;
>+ dri2_surf->out_fence_fd = -1;
>dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> fence_fd);
>
>@@ -263,8 +262,11 @@ static void
> droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf) {
>int ret;
>+ int fence_fd = dri2_surf->out_fence_fd;
>
>- ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
>dri2_surf->buffer, -
>1);
>+ dri2_surf->out_fence_fd = -1;
>+ ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
>+ dri2_surf->buffer, fence_fd);
>if (ret < 0) {
> _eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
> dri2_surf->base.Lost = EGL_TRUE;
>@@ -289,7 +291,7 @@ droid_create_surface(_EGLDriver *drv, _EGLDisplay
>*disp, EGLint type,
> return NULL;
>}
>
>- if (!dri2_init_surface(&dri2_surf->base, disp, type, conf, attrib_list,
>false))
>+ if (!dri2_init_surface(&dri2_surf->base, disp, type, conf,
>+ attrib_list, true))
> goto cleanup_surface;
>
>if (type == EGL_WINDOW_BIT) {
>--
>2.14.1
>
>___
>mesa-dev mailing list
>mesa-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/3] egl: dri3 changes to support surface primitive wrap around dri2
>-Original Message- >From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of >Emil Velikov >Sent: Friday, September 15, 2017 10:00 PM >To: Marathe, Yogesh >Cc: ML mesa-dev >Subject: Re: [Mesa-dev] [PATCH 3/3] egl: dri3 changes to support surface >primitive wrap around dri2 > >On 15 September 2017 at 07:36, wrote: >> From: Yogesh Marathe >> >> As base is moved one level down corresponding implementation in >> dri3 needs a change. >> >> Tested with Intel Mesa CI >> >> Signed-off-by: Yogesh Marathe >> --- >> src/egl/drivers/dri2/platform_x11_dri3.c | 18 +- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >This patch should be squashed with 2/3, and the result must be the first one >in the >series. >Thus as you build and do tests (locally) there is no intermittent >breakage/regression. > >These and more details are part of the Submitting Patches [1] which I'd >encourage you to read through. Sorry I saw this after intermediate reply, I should wait!! So only two patches now 1/2 all changes in platform_x11_dri3. 2/2 all changes in current 1/3. Both being atomically compliable/executable now. I definitely didn’t get this on irc. Thanks. > >Thanks >Emil > >[1] https://www.mesa3d.org/submittingpatches.html Thanks. >___ >mesa-dev mailing list >mesa-dev@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/3] egl: Wrap dri3 surface primitive around dri2 egl surface
Hi Emil, >-Original Message- >From: Emil Velikov [mailto:emil.l.veli...@gmail.com] >Sent: Friday, September 15, 2017 9:52 PM >To: Marathe, Yogesh >Cc: Eric Engestrom ; mesa- >d...@lists.freedesktop.org >Subject: Re: [Mesa-dev] [PATCH 2/3] egl: Wrap dri3 surface primitive around >dri2 >egl surface > >On 15 September 2017 at 16:48, Marathe, Yogesh >wrote: >> Hi Eric, >> >>>-Original Message- >>>From: Eric Engestrom [mailto:eric.engest...@imgtec.com] >>>Sent: Friday, September 15, 2017 7:13 PM >>>To: Marathe, Yogesh >>>Cc: mesa-dev@lists.freedesktop.org >>>Subject: Re: [Mesa-dev] [PATCH 2/3] egl: Wrap dri3 surface primitive >>>around dri2 egl surface >>> >>>On Friday, 2017-09-15 12:06:57 +0530, yogesh.mara...@intel.com wrote: >>>> From: Yogesh Marathe >>>> >>>> Originally dri3 egl surface was wrapped around _EGLSurface. To >>>> support explicit sync, new variables (e.g. enable_out_fence) were >>>> added to dri2_egl_surface. As we reference these new variables we >>>> write on to >>>> dri3 loader bits. These get toggled later in execution due to dri3 >>>> loader. This results in enable_out_fence to have garbage value and >>>> further triggers an assert on dri3 platforms even where fences are >>>> not supported in kernel. >>>> >>>> Thanks to Rafael Antognolli, Emil Velikov and Mark Janes for >>>> catching and root causing this. >>>> >>>> Tested with Intel Mesa CI. >>> >>>I assume you only tested the result of the 3 patches combined, because >>>I'm pretty sure mesa can't compile after patches 1/3 and 2/3: 1/3 >>>makes use of the s/base/surf.base/ change before this patch does that >>>change, and with this patch >>>(2/3) the changes in 3/3 are needed as well. >>> >>>Please run >>>$ git rebase --interactive --exec make origin/master on your branch to >>>make sure each commit compiles. >> >> Ok. Yes I tested the result combined. My assumption was these three >> will always be applied or reverted together. 2/3 and 3/3 can't be >> separated anyways, but I split them based on irc discussion. >> >> I'll run the command you've mentioned so 1/3 will be compliable >> individually and 2/3, 3/3 together. I hope that’s fine. >> >Seems like you've went in the opposite direction to what I mentioned on IRC. >There's a few rules which apply to nearly every project: > - though shalt not intentionally break code, only to fix it with sequential > commit > - though shalt not merge logically separate changes into the same patch My last question on this remained unanswered / was lost in other discussion and I was exactly asking this, how to split it in this case. I thought I can not separate platform_x11_dri3 from 1/3 but your recent reply on 1/3 clarifies. I need to use _eglInitSurface instead of dri2_init_surface. Anyways, now 1/3 will be individually compliable and executable. 2/3 and 3/3 will be together compliable and executable. All changes pertaining to platform_x11_dri3 will be into 2/3 and 3/3. Does this sound ok? > >There's expeptions of course, but on an extremely rare situations. >I'll follow up exactly on each each/how it could be split. > >-Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/3] egl: Wrap dri3 surface primitive around dri2 egl surface
Hi Eric,
>-Original Message-
>From: Eric Engestrom [mailto:eric.engest...@imgtec.com]
>Sent: Friday, September 15, 2017 7:13 PM
>To: Marathe, Yogesh
>Cc: mesa-dev@lists.freedesktop.org
>Subject: Re: [Mesa-dev] [PATCH 2/3] egl: Wrap dri3 surface primitive around
>dri2
>egl surface
>
>On Friday, 2017-09-15 12:06:57 +0530, yogesh.mara...@intel.com wrote:
>> From: Yogesh Marathe
>>
>> Originally dri3 egl surface was wrapped around _EGLSurface. To support
>> explicit sync, new variables (e.g. enable_out_fence) were added to
>> dri2_egl_surface. As we reference these new variables we write on to
>> dri3 loader bits. These get toggled later in execution due to dri3
>> loader. This results in enable_out_fence to have garbage value and
>> further triggers an assert on dri3 platforms even where fences are not
>> supported in kernel.
>>
>> Thanks to Rafael Antognolli, Emil Velikov and Mark Janes for catching
>> and root causing this.
>>
>> Tested with Intel Mesa CI.
>
>I assume you only tested the result of the 3 patches combined, because I'm
>pretty
>sure mesa can't compile after patches 1/3 and 2/3: 1/3 makes use of the
>s/base/surf.base/ change before this patch does that change, and with this
>patch
>(2/3) the changes in 3/3 are needed as well.
>
>Please run
>$ git rebase --interactive --exec make origin/master on your branch to make
>sure
>each commit compiles.
Ok. Yes I tested the result combined. My assumption was these three will always
be
applied or reverted together. 2/3 and 3/3 can't be separated anyways, but I
split
them based on irc discussion.
I'll run the command you've mentioned so 1/3 will be compliable individually
and
2/3, 3/3 together. I hope that’s fine.
>
>>
>> Signed-off-by: Yogesh Marathe
>> ---
>> src/egl/drivers/dri2/platform_x11_dri3.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/egl/drivers/dri2/platform_x11_dri3.h
>> b/src/egl/drivers/dri2/platform_x11_dri3.h
>> index 13d8572..96e7ee9 100644
>> --- a/src/egl/drivers/dri2/platform_x11_dri3.h
>> +++ b/src/egl/drivers/dri2/platform_x11_dri3.h
>> @@ -28,7 +28,7 @@
>> _EGL_DRIVER_TYPECAST(dri3_egl_surface, _EGLSurface, obj)
>>
>> struct dri3_egl_surface {
>> - _EGLSurface base;
>> + struct dri2_egl_surface surf;
>> struct loader_dri3_drawable loader_drawable; };
>>
>> --
>> 2.7.4
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965 : optimized bucket index calculation
>-Original Message- >From: Matt Turner [mailto:matts...@gmail.com] > >On Tue, Sep 12, 2017 at 10:19 AM, Ian Romanick wrote: >> On 09/12/2017 02:40 AM, Marathe, Yogesh wrote: >>> Hi Jason, >>> >>> >>> >>> On the asserts you’ve mentioned below, I assume we need to add them >>> after ‘bufmgr->num_buckets++’ in add_bucket() as num_buckets could be >>> 0 initially. Another clarification on ~1%, we meant approx. 1% there, >>> that’s an improvement we saw in 3Dmark total not a degradation, we’ll >>> correct it in commit msg. >> >> I think the problem is that there is insufficient information about >> your data. What we want to see in a commit message is something like: >> >> commit 5ae2de81c8350272c122ea38e6bb4c0a41d58921 >> Author: Kenneth Graunke >> Date: Mon Aug 28 16:08:32 2017 -0700 >> >> i965: Use BLORP for buffer object stall avoidance blits instead of BLT. >> >> Improves performance of GFXBench4 tests at 1024x768 on a Kabylake GT2: >> - Manhattan 3.1 by 1.32134% +/- 0.322734% (n=8). >> - Car Chase by 1.25607% +/- 0.291262% (n=5). >> >> Reviewed-by: Jason Ekstrand >> >> The important bits are: >> >> - average improvement >> - statistical deviation >> - number of runs > >And for generating such data, we often use http://anholt.net/compare-perf/ Ok. Thanks Matt and Ian, we'll look at it and update commit msg accordingly. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965 : optimized bucket index calculation
Hi Jason,
On the asserts you’ve mentioned below, I assume we need to add them after
‘bufmgr->num_buckets++’ in add_bucket() as num_buckets could be 0 initially.
Another clarification on ~1%, we meant approx. 1% there, that’s an improvement
we saw in 3Dmark total not a degradation, we’ll correct it in commit msg.
Rest all review comments from you, Tapani and Emil are noted & implemented, we
are working on running it through mesa CI/CTS and we should see a v2 for review
after that.
Regards,
Yogesh.
From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of
Jason Ekstrand
Sent: Friday, September 8, 2017 9:09 PM
To: Muthukumar, Aravindan
Cc: mesa-dev@lists.freedesktop.org; J Karanje, Kedar
Subject: Re: [Mesa-dev] [PATCH] i965 : optimized bucket index calculation
In general, I'm very concerned about how this handles rounding behavior.
Almost everywhere, you round down when what you want to do is round up. Also,
as I said on IRC, I'd like to see some asserts in add_bucket so that we are
sure this calculation is correct. In particular, I'd like to see
assert(bucket_for_size(size) == &bufmgr->cache_bucket[i]);
assert(bucket_for_size(size - 2048) == &bufmgr->cache_bucket[i]);
assert(bucket_for_size(size + 1) != &bufmgr->cache_bucket[i]);
We need to check on both sides of size to be 100% sure we're doing our rounding
correctly.
On Fri, Sep 8, 2017 at 1:11 AM,
mailto:aravindan.muthuku...@intel.com>> wrote:
From: Aravindan Muthukumar
mailto:aravindan.muthuku...@intel.com>>
Avoiding the loop which was running with O(n) complexity.
Now the complexity has been reduced to O(1)
Tested with piglit.
Slight performance improvement (~1%) in 3d mark.
Which 3dmark test? Also, what's the error in that 1%?
Change-Id: Id099f1cd24ad5b691a69070eda79b8f4e9be39a6
Signed-off-by: Aravindan Muthukumar
mailto:aravindan.muthuku...@intel.com>>
Signed-off-by: Kedar Karanje
mailto:kedar.j.kara...@intel.com>>
Reviewed-by: Yogesh Marathe
mailto:yogesh.mara...@intel.com>>
---
src/mesa/drivers/dri/i965/brw_bufmgr.c | 48 +-
1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
b/src/mesa/drivers/dri/i965/brw_bufmgr.c
index 5b4e784..18cb166 100644
--- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
@@ -87,6 +87,11 @@
#define memclear(s) memset(&s, 0, sizeof(s))
+/* Macros for BO cache size */
+#define CACHE_PAGE_SIZE4096
Just call this PAGE_SIZE
+#define PAGE_SIZE_SHIFT12
+#define BO_CACHE_PAGE_SIZE (4 * CACHE_PAGE_SIZE)
I think I'd rather we just use 4 * PAGE_SIZE explicitly than have this extra
#define. I think it's making things harder to read and not easier.
+
#define FILE_DEBUG_FLAG DEBUG_BUFMGR
static inline int
@@ -181,19 +186,48 @@ bo_tile_pitch(struct brw_bufmgr *bufmgr, uint32_t pitch,
uint32_t tiling)
return ALIGN(pitch, tile_width);
}
+/*
+ * This functions is to find the correct bucket fit for the input size.
+ * This function works with O(1) complexity when the requested size
+ * was queried instead of iterating the size through all the buckets.
+ */
static struct bo_cache_bucket *
bucket_for_size(struct brw_bufmgr *bufmgr, uint64_t size)
{
- int i;
+ struct bo_cache_bucket *bucket = NULL;
+ int x=0,index = -1;
+ int row, col=0;
- for (i = 0; i < bufmgr->num_buckets; i++) {
- struct bo_cache_bucket *bucket = &bufmgr->cache_bucket[i];
- if (bucket->size >= size) {
- return bucket;
- }
+ /* condition for size less than 4*4096 (4KB) page size */
+ if(size < BO_CACHE_PAGE_SIZE){
This should be "<="
+ index = (size>>PAGE_SIZE_SHIFT)+((size%(1<>PAGE_SIZE_SHIFT;
This rounds down not up like you want.
- return NULL;
+ /* Find the row using Geometric Progression. The highest bit set will
give
+ * the row number. num = a * r^(n-1) where num = size a = 4 r = 2
+ */
+ row = 31 - __builtin_clz(x>>1);
+
+ /* Find the column using AP but using the row value
+ * calculated using GP.
+ */
+ col =((x-(1<<(row+1)))/(1<<(row-1)))+1;
+ col += (size%(1<= 0 && index <
bufmgr->num_buckets)?(&bufmgr->cache_bucket[index]):NULL;
+ return bucket;
}
int
--
2.7.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out fence
> -Original Message- > From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > Sent: Friday, September 8, 2017 11:03 PM > To: Antognolli, Rafael > >> > > Isn't this strange? Can someone please comment? > >> > > > >> > In all fairness there was a few wtf moments as Mark mentioned the > >> > issue. As on a quick look "it cannot happen" :-\ > >> > > >> > One way is to add some printfs "debugging" across the board and > >> > check with Mark if he can run (only?) the affected test on the CI. > >> > > >> > >> Number of tests failing on CI due to this are huge, any 'one' can be > >> picked up. I do have my CI branch setup now but I don’t think I can > >> use it for debugging (not advised). I'll sync up with Mark again. Just > >> wanted a > confirmation, I'm not missing something obvious. Thanks. > > > > Hi Yogesh, > > > > I replied to you already when you messaged in private, the error is > > not related to the kernel returning true for that, it's related to a > > memory corruption caused by wrong use of the dri2_surf_init inside > > platform_x11_dri3.c. Quoting myself: > > > > "More specifically, it looks like this test fails every time: > > > > glcts -n > > dEQP-EGL.functional.query_context.get_current_context.rgb888_window > > This passes for me. That’s what confused me again. how is that possible? Output: Writing test log into TestResults.qpa dEQP Core git-dfcb8e870438f6f2bfe71d4bb63d43120debb3a3 (0xdfcb8e87) starting.. target implementation = 'X11 EGL' Test case 'dEQP-EGL.functional.query_context.get_current_context.rgb888_window'.. Pass (Pass) DONE! Test run totals: Passed:1/1 (100.0%) Failed:0/1 (0.0%) Not supported: 0/1 (0.0%) Warnings: 0/1 (0.0%) > > I see several valgrind warnings inside platform_x11_dri3.c. I believe > > you are probably accessing the dri2_surf before it was allocated, or > > after it was freed..." > > > > When I tested this back then, the "out_fence_enable" (or whatever was > > called) in dri2 was false, but after a couple runs it would become a > > bogus number, which also points to memory corruption. > > > > I suggest ignoring the kernel and focusing on valgrind debugging. > > > Nicely spotted there Rafael. > > The issue is that the dri3 surface primitive wraps around _EGLSurface. > Thus as we reference the new variables we effectively write onto the > loader_dri3 bits. And at a later stage the dri3 loader code toggles those to > "use > out fence = true". I will check this with valgrind. > > -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out fence
> -Original Message- > From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > Sent: Friday, September 8, 2017 8:28 PM > To: Marathe, Yogesh > Cc: Tomasz Figa ; Antognolli, Rafael > ; Janes, Mark A ; > mesa-dev@lists.freedesktop.org; Gao, Shuo ; Liu, > Zhiquan ; dani...@collabora.com; > nicolai.haeh...@amd.com; e...@engestrom.ch; Wu, Zhongmin > ; kenn...@whitecape.org; Kondapally, Kalyan > ; fernetme...@online.de; > tarc...@itsqueeze.com; varad.gau...@collabora.com > Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out > fence > > On 8 September 2017 at 14:47, Marathe, Yogesh > wrote: > > Hello Folks, > > > > Sorry for late reply, I took quite some time to CTS up, comments below. > > > >> -Original Message- > >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > >> Behalf Of Marathe, Yogesh > >> Sent: Friday, September 1, 2017 10:16 AM > >> To: Tomasz Figa > >> Cc: Gao, Shuo ; Liu, Zhiquan > >> ; dani...@collabora.com; > >> nicolai.haeh...@amd.com; Antognolli, Rafael > >> ; e...@engestrom.ch; Emil Velikov > >> ; Wu, Zhongmin ; > >> kenn...@whitecape.org; Kondapally, Kalyan > >> ; fernetme...@online.de; > >> mesa-dev@lists.freedesktop.org; tarc...@itsqueeze.com; > >> varad.gau...@collabora.com > >> Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per > >> surface out fence > >> > >> Tomasz, > >> > >> > -Original Message- > >> > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > >> > Behalf Of Tomasz Figa > >> > Sent: Friday, September 1, 2017 9:53 AM > >> > To: Marathe, Yogesh On Thu, Aug 31, 2017 > >> > at 2:18 AM, Marathe, Yogesh wrote: > >> > >> -Original Message- > >> > >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] > >> > >> On Behalf Of Emil Velikov > >> > >> Sent: Wednesday, August 30, 2017 9:44 PM > >> > >> To: Marathe, Yogesh > >> > >> Cc: Gao, Shuo ; Liu, Zhiquan > >> > >> ; dani...@collabora.com; > >> > >> nicolai.haeh...@amd.com; Antognolli, Rafael > >> > >> ; e...@engestrom.ch; Wu, Zhongmin > >> > >> ; tf...@chromium.org; > >> kenn...@whitecape.org; > >> > >> Kondapally, Kalyan ; > >> > >> fernetme...@online.de; mesa-dev@lists.freedesktop.org; > >> > >> tarc...@itsqueeze.com; varad.gau...@collabora.com > >> > >> Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per > >> > >> surface out fence > >> > >> > >> > >> On 30 August 2017 at 15:39, Marathe, Yogesh > >> > >> > >> > >> wrote: > >> > >> > >> > >> > > >> > >> > Thank you, Tomasz and all involved for the help and guidance. > >> > >> > > >> > >> Our excitement was short lived - see commit > >> > >> 8c9df0daf20206fafb7df77b1edcbc41b8e91372. > >> > >> > >> > >> Seems the patch was not run through the Intel CI, though I'm > >> > >> should not have assumed that you're aware of if. > >> > >> Please get in touch with Mark Janes (Cc'd here, janesma on IRC). > >> > >> He can set you up and/or run a branch for you. > >> > >> > >> > > > >> > > No problem. I will contact Mark. > >> > > > >> > > Primarily looks like platform / kernel version issue. > >> > > intel_get_boolean() for I915_PARAM_HAS_EXEC_FENCE is false, but I > >> > > see following in i915_drv.c:915_getparam in kernel, no clue why > >> > > that would come false in UMD. > >> > > > >> > > ... > >> > > case I915_PARAM_HAS_EXEC_FENCE: > >> > > /* For the time being all of these are always true; > >> > > * if some supported hardware does not have one of > >> > > these > >> > > * features this value needs to be provided from > >> > > * INTEL_INFO(), a feature macro, or similar. > >> > > */ > >> > > value = 1; > >> > > break; > >> > > ... > >> > > >>
Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out fence
Hello Folks, Sorry for late reply, I took quite some time to CTS up, comments below. > -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > Of Marathe, Yogesh > Sent: Friday, September 1, 2017 10:16 AM > To: Tomasz Figa > Cc: Gao, Shuo ; Liu, Zhiquan ; > dani...@collabora.com; nicolai.haeh...@amd.com; Antognolli, Rafael > ; e...@engestrom.ch; Emil Velikov > ; Wu, Zhongmin ; > kenn...@whitecape.org; Kondapally, Kalyan ; > fernetme...@online.de; mesa-dev@lists.freedesktop.org; > tarc...@itsqueeze.com; varad.gau...@collabora.com > Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out > fence > > Tomasz, > > > -Original Message- > > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > > Behalf Of Tomasz Figa > > Sent: Friday, September 1, 2017 9:53 AM > > To: Marathe, Yogesh > > On Thu, Aug 31, 2017 at 2:18 AM, Marathe, Yogesh > > wrote: > > >> -Original Message- > > >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > > >> Behalf Of Emil Velikov > > >> Sent: Wednesday, August 30, 2017 9:44 PM > > >> To: Marathe, Yogesh > > >> Cc: Gao, Shuo ; Liu, Zhiquan > > >> ; dani...@collabora.com; > > >> nicolai.haeh...@amd.com; Antognolli, Rafael > > >> ; e...@engestrom.ch; Wu, Zhongmin > > >> ; tf...@chromium.org; > kenn...@whitecape.org; > > >> Kondapally, Kalyan ; > > >> fernetme...@online.de; mesa-dev@lists.freedesktop.org; > > >> tarc...@itsqueeze.com; varad.gau...@collabora.com > > >> Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per > > >> surface out fence > > >> > > >> On 30 August 2017 at 15:39, Marathe, Yogesh > > >> > > >> wrote: > > >> > > >> > > > >> > Thank you, Tomasz and all involved for the help and guidance. > > >> > > > >> Our excitement was short lived - see commit > > >> 8c9df0daf20206fafb7df77b1edcbc41b8e91372. > > >> > > >> Seems the patch was not run through the Intel CI, though I'm should > > >> not have assumed that you're aware of if. > > >> Please get in touch with Mark Janes (Cc'd here, janesma on IRC). He > > >> can set you up and/or run a branch for you. > > >> > > > > > > No problem. I will contact Mark. > > > > > > Primarily looks like platform / kernel version issue. > > > intel_get_boolean() for I915_PARAM_HAS_EXEC_FENCE is false, but I > > > see following in i915_drv.c:915_getparam in kernel, no clue why that > > > would come false in UMD. > > > > > > ... > > > case I915_PARAM_HAS_EXEC_FENCE: > > > /* For the time being all of these are always true; > > > * if some supported hardware does not have one of these > > > * features this value needs to be provided from > > > * INTEL_INFO(), a feature macro, or similar. > > > */ > > > value = 1; > > > break; > > > ... > > > > Which kernel are you looking at? Remember that not everyone uses > > current upstream master. There is a number of upstream stable releases > > and downstream forks. Grepping for I915_PARAM_HAS_EXEC_FENCE on > > http://elixir.free-electrons.com, shows that it was only added in Linux > > 4.12. > > > > I'm on 4.9.x but I see my kernel tree has following patch, so this looks > like it is > done for android (cherry picked / backport). That’s why it worked for me > always! > > commit f0683754f03fa308a2657cb1dadbf235c9607188 > Author: Chris Wilson > Date: Fri Jan 27 09:40:08 2017 + > > drm/i915: Support explicit fencing for execbuf > > Nonetheless, as you mentioned, I've synced up with Mark and we've created a > separate branch where CTS / intel mesa CI can run. Let me try to fix this. > > Caveat: To have flatland running on android there was another issue in kernel > which needed a fix. Details - > https://bugs.freedesktop.org/show_bug.cgi?id=101656 I was able to run CTS (https://github.com/KhronosGroup/VK-GL-CTS) on this patch for x11_egl. I see exact same results before and after patch on Ubuntu 16.04 setup. Mark had also mentioned this works fine on 4.12 onwards (essentially with drm/i915: Support explicit fencing for execbuf patch in kernel). Rega
Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out fence
Tomasz, > -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > Of Tomasz Figa > Sent: Friday, September 1, 2017 9:53 AM > To: Marathe, Yogesh > Cc: Gao, Shuo ; Liu, Zhiquan ; > dani...@collabora.com; nicolai.haeh...@amd.com; Antognolli, Rafael > ; e...@engestrom.ch; Emil Velikov > ; Wu, Zhongmin ; > kenn...@whitecape.org; mesa-dev@lists.freedesktop.org; > fernetme...@online.de; Kondapally, Kalyan ; > tarc...@itsqueeze.com; varad.gau...@collabora.com > Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out > fence > > On Thu, Aug 31, 2017 at 2:18 AM, Marathe, Yogesh > wrote: > >> -Original Message- > >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > >> Behalf Of Emil Velikov > >> Sent: Wednesday, August 30, 2017 9:44 PM > >> To: Marathe, Yogesh > >> Cc: Gao, Shuo ; Liu, Zhiquan > >> ; dani...@collabora.com; > >> nicolai.haeh...@amd.com; Antognolli, Rafael > >> ; e...@engestrom.ch; Wu, Zhongmin > >> ; tf...@chromium.org; kenn...@whitecape.org; > >> Kondapally, Kalyan ; > >> fernetme...@online.de; mesa-dev@lists.freedesktop.org; > >> tarc...@itsqueeze.com; varad.gau...@collabora.com > >> Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per > >> surface out fence > >> > >> On 30 August 2017 at 15:39, Marathe, Yogesh > >> > >> wrote: > >> > >> > > >> > Thank you, Tomasz and all involved for the help and guidance. > >> > > >> Our excitement was short lived - see commit > >> 8c9df0daf20206fafb7df77b1edcbc41b8e91372. > >> > >> Seems the patch was not run through the Intel CI, though I'm should > >> not have assumed that you're aware of if. > >> Please get in touch with Mark Janes (Cc'd here, janesma on IRC). He > >> can set you up and/or run a branch for you. > >> > > > > No problem. I will contact Mark. > > > > Primarily looks like platform / kernel version issue. > > intel_get_boolean() for I915_PARAM_HAS_EXEC_FENCE is false, but I see > > following in i915_drv.c:915_getparam in kernel, no clue why that would > > come false in UMD. > > > > ... > > case I915_PARAM_HAS_EXEC_FENCE: > > /* For the time being all of these are always true; > > * if some supported hardware does not have one of these > > * features this value needs to be provided from > > * INTEL_INFO(), a feature macro, or similar. > > */ > > value = 1; > > break; > > ... > > Which kernel are you looking at? Remember that not everyone uses current > upstream master. There is a number of upstream stable releases and > downstream forks. Grepping for I915_PARAM_HAS_EXEC_FENCE on > http://elixir.free-electrons.com, shows that it was only added in Linux 4.12. > I'm on 4.9.x but I see my kernel tree has following patch, so this looks like it is done for android (cherry picked / backport). That’s why it worked for me always! commit f0683754f03fa308a2657cb1dadbf235c9607188 Author: Chris Wilson Date: Fri Jan 27 09:40:08 2017 + drm/i915: Support explicit fencing for execbuf Nonetheless, as you mentioned, I've synced up with Mark and we've created a separate branch where CTS / intel mesa CI can run. Let me try to fix this. Caveat: To have flatland running on android there was another issue in kernel which needed a fix. Details - https://bugs.freedesktop.org/show_bug.cgi?id=101656 > Best regards, > Tomasz > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out fence
> -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > Of Emil Velikov > Sent: Wednesday, August 30, 2017 9:44 PM > To: Marathe, Yogesh > Cc: Gao, Shuo ; Liu, Zhiquan ; > dani...@collabora.com; nicolai.haeh...@amd.com; Antognolli, Rafael > ; e...@engestrom.ch; Wu, Zhongmin > ; tf...@chromium.org; kenn...@whitecape.org; > Kondapally, Kalyan ; fernetme...@online.de; > mesa-dev@lists.freedesktop.org; tarc...@itsqueeze.com; > varad.gau...@collabora.com > Subject: Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out > fence > > On 30 August 2017 at 15:39, Marathe, Yogesh > wrote: > > > > > Thank you, Tomasz and all involved for the help and guidance. > > > Our excitement was short lived - see commit > 8c9df0daf20206fafb7df77b1edcbc41b8e91372. > > Seems the patch was not run through the Intel CI, though I'm should not have > assumed that you're aware of if. > Please get in touch with Mark Janes (Cc'd here, janesma on IRC). He can set > you > up and/or run a branch for you. > No problem. I will contact Mark. Primarily looks like platform / kernel version issue. intel_get_boolean() for I915_PARAM_HAS_EXEC_FENCE is false, but I see following in i915_drv.c:915_getparam in kernel, no clue why that would come false in UMD. ... case I915_PARAM_HAS_EXEC_FENCE: /* For the time being all of these are always true; * if some supported hardware does not have one of these * features this value needs to be provided from * INTEL_INFO(), a feature macro, or similar. */ value = 1; break; ... > Thanks > Emil > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out fence
> -Original Message- > From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > Sent: Wednesday, August 30, 2017 4:39 PM > To: Marathe, Yogesh > Cc: mesa-dev@lists.freedesktop.org; tf...@chromium.org; Gao, Shuo > ; Liu, Zhiquan ; > dani...@collabora.com; nicolai.haeh...@amd.com; Antognolli, Rafael > ; e...@engestrom.ch; kenn...@whitecape.org; > fernetme...@online.de; Kondapally, Kalyan ; > tarc...@itsqueeze.com; varad.gau...@collabora.com; Wu, Zhongmin > > Subject: Re: [PATCH v6.2] egl: Allow creation of per surface out fence > > On 28 August 2017 at 11:27, Marathe, Yogesh > wrote: > > This still doesn't seem to be merged. Can someone please look at it? It does > have Rbs. > > > Done. Thanks for the work and patience on getting this sorted. You're welcome. Thank you, Tomasz and all involved for the help and guidance. Regards, Yogesh. > > -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v6.2] egl: Allow creation of per surface out fence
This still doesn't seem to be merged. Can someone please look at it? It does
have Rbs.
Regards,
Yogesh.
> -Original Message-
> From: Marathe, Yogesh
> Sent: Wednesday, August 23, 2017 11:35 PM
> To: mesa-dev@lists.freedesktop.org
> Cc: tf...@chromium.org; emil.l.veli...@gmail.com; Gao, Shuo
> ; Liu, Zhiquan ;
> dani...@collabora.com; nicolai.haeh...@amd.com; Antognolli, Rafael
> ; e...@engestrom.ch; kenn...@whitecape.org;
> fernetme...@online.de; Kondapally, Kalyan ;
> tarc...@itsqueeze.com; varad.gau...@collabora.com; Wu, Zhongmin
> ; Marathe, Yogesh
> Subject: [PATCH v6.2] egl: Allow creation of per surface out fence
>
> From: Zhongmin Wu
>
> Add plumbing to allow creation of per display surface out fence.
>
> Currently enabled only on android, since the system expects a valid fd in
> ANativeWindow::{queue,cancel}Buffer. We pass a fd of -1 with which native
> applications such as flatland fail. The patch enables explicit sync on
> android and
> fixes one of the functional issue for apps or buffer consumers which depend
> upon fence and its timestamp.
>
> v2: a) Also implement the fence in cancelBuffer.
> b) The last sync fence is stored in drawable object
>rather than brw context.
> c) format clear.
>
> v3: a) Save the last fence fd in DRI Context object.
> b) Return the last fence if the batch buffer is empty and
>nothing to be flushed when _intel_batchbuffer_flush_fence
> c) Add the new interface in vbtl to set the retrieve fence
>
> v3.1 a) close fd in the new vbtl interface on none Android platform
>
> v4: a) The last fence is saved in brw context.
> b) The retrieve fd is for all the platform but not just Android
> c) Add a uniform dri2 interface to initialize the surface.
>
> v4.1: a) make some changes of variable name.
> b) the patch is broken into two patches.
>
> v4.2: a) Add a deinit interface for surface to clear the out fence
>
> v5: a) Add enable_out_fence to init, platform sets it true or
>false
> b) Change get fd to update fd and check for fence
> c) Commit description updated
>
> v6: a) Heading and commit description updated
> b) enable_out_fence is set only if fence is supported
> c) Review comments on function names
> d) Test with standalone patch, resolves the bug
>
> v6.1: Check for old display fence reverted
>
> v6.2: enable_out_fence initialized to false by default,
> dri2_surf_update_fence_fd updated, deinit changed to fini
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101655
>
> Signed-off-by: Zhongmin Wu
> Signed-off-by: Yogesh Marathe
> Reviewed-by: Emil Velikov
> Reviewed-by: Tomasz Figa
> ---
> src/egl/drivers/dri2/egl_dri2.c | 71
> +
> src/egl/drivers/dri2/egl_dri2.h | 9
> src/egl/drivers/dri2/platform_android.c | 29 ++--
> src/egl/drivers/dri2/platform_drm.c | 3 +-
> src/egl/drivers/dri2/platform_surfaceless.c | 3 +-
> src/egl/drivers/dri2/platform_wayland.c | 3 +-
> src/egl/drivers/dri2/platform_x11.c | 3 +-
> src/egl/drivers/dri2/platform_x11_dri3.c| 3 +-
> 8 files changed, 106 insertions(+), 18 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c
> b/src/egl/drivers/dri2/egl_dri2.c index
> aa6f02a..44b8e1d 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1388,6 +1388,45 @@ dri2_destroy_context(_EGLDriver *drv, _EGLDisplay
> *disp, _EGLContext *ctx)
> return EGL_TRUE;
> }
>
> +EGLBoolean
> +dri2_init_surface(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> +_EGLConfig *conf, const EGLint *attrib_list, EGLBoolean
> +enable_out_fence) {
> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> +
> + dri2_surf->out_fence_fd = -1;
> + dri2_surf->enable_out_fence = false;
> + if (dri2_dpy->fence && dri2_dpy->fence->base.version >= 2 &&
> + dri2_dpy->fence->get_capabilities &&
> + (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
> +__DRI_FENCE_CAP_NATIVE_FD)) {
> + dri2_surf->enable_out_fence = enable_out_fence;
> + }
> +
> + return _eglInitSurface(surf, dpy, type, conf, attrib_list); }
> +
> +static void
> +dri2_surface_set_out_fence_fd( _EGLSurface *surf, int fence_fd) {
> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> +
> + if (dri2_surf->out_fence_fd >=0)
> + close(dri2_surf->out_fence_fd);
> +
> + dri2_sur
Re: [Mesa-dev] [PATCH v6.1] egl: Allow creation of per surface out fence
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Wednesday, August 23, 2017 8:17 PM
> To: Marathe, Yogesh
> Cc: ML mesa-dev ; Emil Velikov
> ; Gao, Shuo ; Liu, Zhiquan
> ; Daniel Stone ; Nicolai
> Hähnle ; Antognolli, Rafael
> ; Eric Engestrom ; Kenneth
> Graunke ; Rainer Hochecker
> ; Kondapally, Kalyan ;
> Timothy Arceri ; Varad Gautam
> ; Wu, Zhongmin
> Subject: Re: [PATCH v6.1] egl: Allow creation of per surface out fence
>
> Hi Yogesh,
>
> Sorry for being late with review. Please see some comments inline.
>
No problem.
> On Fri, Aug 18, 2017 at 7:08 PM, wrote:
> > From: Zhongmin Wu
> >
> > Add plumbing to allow creation of per display surface out fence.
> >
> > Currently enabled only on android, since the system expects a valid fd
> > in ANativeWindow::{queue,cancel}Buffer. We pass a fd of -1 with which
> > native applications such as flatland fail. The patch enables explicit
> > sync on android and fixes one of the functional issue for apps or
> > buffer consumers which depend upon fence and its timestamp.
> >
> > v2: a) Also implement the fence in cancelBuffer.
> > b) The last sync fence is stored in drawable object
> >rather than brw context.
> > c) format clear.
> >
> > v3: a) Save the last fence fd in DRI Context object.
> > b) Return the last fence if the batch buffer is empty and
> >nothing to be flushed when _intel_batchbuffer_flush_fence
> > c) Add the new interface in vbtl to set the retrieve fence
> >
> > v3.1 a) close fd in the new vbtl interface on none Android platform
> >
> > v4: a) The last fence is saved in brw context.
> > b) The retrieve fd is for all the platform but not just Android
> > c) Add a uniform dri2 interface to initialize the surface.
> >
> > v4.1: a) make some changes of variable name.
> > b) the patch is broken into two patches.
> >
> > v4.2: a) Add a deinit interface for surface to clear the out fence
> >
> > v5: a) Add enable_out_fence to init, platform sets it true or
> >false
> > b) Change get fd to update fd and check for fence
> > c) Commit description updated
> >
> > v6: a) Heading and commit description updated
> > b) enable_out_fence is set only if fence is supported
> > c) Review comments on function names
> > d) Test with standalone patch, resolves the bug
> >
> > v6.1 a) Check for old display fence reverted back
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101655
> >
> > Signed-off-by: Zhongmin Wu
> > Signed-off-by: Yogesh Marathe
> > ---
> > src/egl/drivers/dri2/egl_dri2.c | 69
> > +
> > src/egl/drivers/dri2/egl_dri2.h | 9
> > src/egl/drivers/dri2/platform_android.c | 29 ++--
> > src/egl/drivers/dri2/platform_drm.c | 3 +-
> > src/egl/drivers/dri2/platform_surfaceless.c | 3 +-
> > src/egl/drivers/dri2/platform_wayland.c | 3 +-
> > src/egl/drivers/dri2/platform_x11.c | 3 +-
> > src/egl/drivers/dri2/platform_x11_dri3.c| 3 +-
> > 8 files changed, 104 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/egl_dri2.c
> > b/src/egl/drivers/dri2/egl_dri2.c index ed79e0d..04d0332 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.c
> > +++ b/src/egl/drivers/dri2/egl_dri2.c
> > @@ -1354,6 +1354,44 @@ dri2_destroy_context(_EGLDriver *drv,
> _EGLDisplay *disp, _EGLContext *ctx)
> > return EGL_TRUE;
> > }
> >
> > +EGLBoolean
> > +dri2_init_surface(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> > +_EGLConfig *conf, const EGLint *attrib_list, EGLBoolean
> > +enable_out_fence) {
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> > +
> > + dri2_surf->out_fence_fd = -1;
> > + if (dri2_dpy->fence && dri2_dpy->fence->base.version >= 2 &&
> > + dri2_dpy->fence->get_capabilities &&
> > + (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
> > +__DRI_FENCE_CAP_NATIVE_FD)) {
> > + dri2_surf->enable_out_fence = enable_out_fence;
> > + }
>
> nit: It might not change anything in practice, but it would be more logical
> if the
> code always initialized enable_out_fence to some value.
> So maybe let's add dri2_surf->enable_out_fence = 0; above the if.
>
Re: [Mesa-dev] [PATCH v6.1] egl: Allow creation of per surface out fence
> -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > Of Emil Velikov > Sent: Monday, August 21, 2017 11:24 PM > To: Marathe, Yogesh > Cc: Gao, Shuo ; Liu, Zhiquan ; > dani...@collabora.com; nicolai.haeh...@amd.com; Antognolli, Rafael > ; e...@engestrom.ch; Wu, Zhongmin > ; tf...@chromium.org; kenn...@whitecape.org; > Kondapally, Kalyan ; fernetme...@online.de; > mesa-dev@lists.freedesktop.org; tarc...@itsqueeze.com; > varad.gau...@collabora.com > Subject: Re: [Mesa-dev] [PATCH v6.1] egl: Allow creation of per surface out > fence > > On 21 August 2017 at 16:45, Marathe, Yogesh > wrote: > > Can someone please comment on this? Are we done / still not done here? > > > I would have s/deinit/fini/ but we can squash that on merge or at a later > point. > > Regardless of the above: > Reviewed-by: Emil Velikov Thank you Emil. Will take care of s/deinit/fini/ separately. I saw that as a comment earlier but 'fini' didn't make lot of sense to me so I left it to deinit. Sorry I should've asked this. Now I see other fini functions around & it's clear! I will also update the bugzilla. > > -Emil > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v6.1] egl: Allow creation of per surface out fence
Can someone please comment on this? Are we done / still not done here?
Regards,
Yogesh.
> -Original Message-
> From: Marathe, Yogesh
> Sent: Friday, August 18, 2017 3:39 PM
> To: mesa-dev@lists.freedesktop.org
> Cc: tf...@chromium.org; emil.l.veli...@gmail.com; Gao, Shuo
> ; Liu, Zhiquan ;
> dani...@collabora.com; nicolai.haeh...@amd.com; Antognolli, Rafael
> ; e...@engestrom.ch; kenn...@whitecape.org;
> fernetme...@online.de; Kondapally, Kalyan ;
> tarc...@itsqueeze.com; varad.gau...@collabora.com; Wu, Zhongmin
> ; Marathe, Yogesh
> Subject: [PATCH v6.1] egl: Allow creation of per surface out fence
>
> From: Zhongmin Wu
>
> Add plumbing to allow creation of per display surface out fence.
>
> Currently enabled only on android, since the system expects a valid fd in
> ANativeWindow::{queue,cancel}Buffer. We pass a fd of -1 with which native
> applications such as flatland fail. The patch enables explicit sync on
> android and
> fixes one of the functional issue for apps or buffer consumers which depend
> upon fence and its timestamp.
>
> v2: a) Also implement the fence in cancelBuffer.
> b) The last sync fence is stored in drawable object
>rather than brw context.
> c) format clear.
>
> v3: a) Save the last fence fd in DRI Context object.
> b) Return the last fence if the batch buffer is empty and
>nothing to be flushed when _intel_batchbuffer_flush_fence
> c) Add the new interface in vbtl to set the retrieve fence
>
> v3.1 a) close fd in the new vbtl interface on none Android platform
>
> v4: a) The last fence is saved in brw context.
> b) The retrieve fd is for all the platform but not just Android
> c) Add a uniform dri2 interface to initialize the surface.
>
> v4.1: a) make some changes of variable name.
> b) the patch is broken into two patches.
>
> v4.2: a) Add a deinit interface for surface to clear the out fence
>
> v5: a) Add enable_out_fence to init, platform sets it true or
>false
> b) Change get fd to update fd and check for fence
> c) Commit description updated
>
> v6: a) Heading and commit description updated
> b) enable_out_fence is set only if fence is supported
> c) Review comments on function names
> d) Test with standalone patch, resolves the bug
>
> v6.1 a) Check for old display fence reverted back
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101655
>
> Signed-off-by: Zhongmin Wu
> Signed-off-by: Yogesh Marathe
> ---
> src/egl/drivers/dri2/egl_dri2.c | 69
> +
> src/egl/drivers/dri2/egl_dri2.h | 9
> src/egl/drivers/dri2/platform_android.c | 29 ++--
> src/egl/drivers/dri2/platform_drm.c | 3 +-
> src/egl/drivers/dri2/platform_surfaceless.c | 3 +-
> src/egl/drivers/dri2/platform_wayland.c | 3 +-
> src/egl/drivers/dri2/platform_x11.c | 3 +-
> src/egl/drivers/dri2/platform_x11_dri3.c| 3 +-
> 8 files changed, 104 insertions(+), 18 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c
> b/src/egl/drivers/dri2/egl_dri2.c index
> ed79e0d..04d0332 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1354,6 +1354,44 @@ dri2_destroy_context(_EGLDriver *drv, _EGLDisplay
> *disp, _EGLContext *ctx)
> return EGL_TRUE;
> }
>
> +EGLBoolean
> +dri2_init_surface(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> +_EGLConfig *conf, const EGLint *attrib_list, EGLBoolean
> +enable_out_fence) {
> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> +
> + dri2_surf->out_fence_fd = -1;
> + if (dri2_dpy->fence && dri2_dpy->fence->base.version >= 2 &&
> + dri2_dpy->fence->get_capabilities &&
> + (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
> +__DRI_FENCE_CAP_NATIVE_FD)) {
> + dri2_surf->enable_out_fence = enable_out_fence;
> + }
> +
> + return _eglInitSurface(surf, dpy, type, conf, attrib_list); }
> +
> +static void
> +dri2_surface_set_out_fence_fd( _EGLSurface *surf, int fence_fd) {
> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> +
> + if (dri2_surf->out_fence_fd >=0)
> + close(dri2_surf->out_fence_fd);
> +
> + dri2_surf->out_fence_fd = fence_fd;
> +}
> +
> +void
> +dri2_deinit_surface(_EGLSurface *surf)
> +{
> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> +
> + dri2_surface_set_out_fence_fd(surf, -1);
> + dri2_surf->ena
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
Tomasz, > -Original Message- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Thursday, August 10, 2017 5:29 PM > To: Marathe, Yogesh > Hi Yogesh, > > On Thu, Aug 10, 2017 at 5:25 PM, Marathe, Yogesh > wrote: > > Hi Tomasz, > > > >> -Original Message- > >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > >> Behalf Of Tomasz Figa > >> Sent: Tuesday, August 8, 2017 7:45 AM > >> To: Marathe, Yogesh > >> >> > >> Changing the topic, the patch doesn't seem to change the > >> >> > >> implementation of swapBuffers to stop doing a flush on the > >> >> > >> buffer, which defeats the purpose of the fence, as the it is > >> >> > >> likely already signaled at the time it is passed to > >> >> > >> queueBuffer. Shouldn't > >> we fix this? > >> >> > >> > >> >> > > > >> >> > > I have been wondering about it all the while, when I had > >> >> > > prints in > >> >> > > Fence::getSignalTime() to check finfo->status from consumer > >> >> > > side during initial revisions, I always found it to be signaled! > >> >> > > > >> >> > > Can we really remove that flush in swapBuffers? In that case I > >> >> > > believe the consumer _must_ wait on fence before really > >> >> > > accessing it, so that would trigger a change in buffer consumer / > application! > >> >> > > >> >> > The consumer must _always_ wait on the acquire fence if it's a > >> >> > valid FD, as this is how the ANativeWindow interface is defined. > >> >> > You can see Mesa already does it in droid_dequeue_buffer(). If > >> >> > you find a consumer that is not doing so, it's a bug in the consumer. > >> >> > There is no compatibility concern here, as it's strictly > >> >> > regulated by Android > >> specifications. > >> >> > >> >> I checked this, yes, BufferConsumer waits on fence provided its valid. > >> > > >> > Hi Tomasz, > >> > > >> > Is it ok to move that 'flush' removal change to separate commit? I > >> > would opt for that. This gflush removal change is going to trigger > >> > additional tests, while this one fixes the issue for now and has > >> > list of review comments done. If this is fine, I'll push v6 for this. > >> > >> I'm okay with either. > >> > > > > I found GLConsumer aosp has glFlush() is already, it means we had two > > flush calls in the path, one in swapBuffers and other in libgui on consumer. > > > > I went ahead and removed dri2_flush_drawable_for_swapbuffer. > > Functionally, things seem to be ok. I assume this will be valid only > > for android with valid fence changes and not for other platforms. Is this > > right > expectation? Diff below. > > > > diff --git a/src/egl/drivers/dri2/platform_android.c > > b/src/egl/drivers/dri2/platform_android.c > > index 8bca753..80da021 100644 > > --- a/src/egl/drivers/dri2/platform_android.c > > +++ b/src/egl/drivers/dri2/platform_android.c > > @@ -706,7 +706,6 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay > *disp, _EGLSurface *draw) > > if (dri2_surf->back) > >dri2_surf->back->age = 1; > > > > - dri2_flush_drawable_for_swapbuffers(disp, draw); > > > > /* dri2_surf->buffer can be null even when no error has occured. For > > * example, if the user has called no GL rendering commands since > > the > > > > If this is only change, I don’t think we need separate patch here. Please > > correct > me if I'm wrong. > > I think I have been mistaken in what > dri2_flush_drawable_for_swabuffers() does. We need to keep it there as it > makes the DRI2 driver issue operations finalizing the buffer, e.g. > remaining drawing or a multisample resolve if necessary. However it doesn't do > any synchronous wait on the queued operations, so there is no performance > loss. > I am OK either ways. Another thing, glFlush in libgui seems legitimate, it is required in order to have eglDupNativeFenceFDANDROID() return a valid fd! Now only pending thing is the 'old display surface' fence then, and we have decided to wait for someone else from android to comment on it. > Best regards, > Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
Hi Tomasz, > -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > Of Tomasz Figa > Sent: Tuesday, August 8, 2017 7:45 AM > To: Marathe, Yogesh > >> > >> Changing the topic, the patch doesn't seem to change the > >> > >> implementation of swapBuffers to stop doing a flush on the > >> > >> buffer, which defeats the purpose of the fence, as the it is > >> > >> likely already signaled at the time it is passed to queueBuffer. > >> > >> Shouldn't > we fix this? > >> > >> > >> > > > >> > > I have been wondering about it all the while, when I had prints > >> > > in > >> > > Fence::getSignalTime() to check finfo->status from consumer side > >> > > during initial revisions, I always found it to be signaled! > >> > > > >> > > Can we really remove that flush in swapBuffers? In that case I > >> > > believe the consumer _must_ wait on fence before really accessing > >> > > it, so that would trigger a change in buffer consumer / application! > >> > > >> > The consumer must _always_ wait on the acquire fence if it's a > >> > valid FD, as this is how the ANativeWindow interface is defined. > >> > You can see Mesa already does it in droid_dequeue_buffer(). If you > >> > find a consumer that is not doing so, it's a bug in the consumer. > >> > There is no compatibility concern here, as it's strictly regulated by > >> > Android > specifications. > >> > >> I checked this, yes, BufferConsumer waits on fence provided its valid. > > > > Hi Tomasz, > > > > Is it ok to move that 'flush' removal change to separate commit? I > > would opt for that. This gflush removal change is going to trigger > > additional tests, while this one fixes the issue for now and has list > > of review comments done. If this is fine, I'll push v6 for this. > > I'm okay with either. > I found GLConsumer aosp has glFlush() is already, it means we had two flush calls in the path, one in swapBuffers and other in libgui on consumer. I went ahead and removed dri2_flush_drawable_for_swapbuffer. Functionally, things seem to be ok. I assume this will be valid only for android with valid fence changes and not for other platforms. Is this right expectation? Diff below. diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c index 8bca753..80da021 100644 --- a/src/egl/drivers/dri2/platform_android.c +++ b/src/egl/drivers/dri2/platform_android.c @@ -706,7 +706,6 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw) if (dri2_surf->back) dri2_surf->back->age = 1; - dri2_flush_drawable_for_swapbuffers(disp, draw); /* dri2_surf->buffer can be null even when no error has occured. For * example, if the user has called no GL rendering commands since the If this is only change, I don’t think we need separate patch here. Please correct me if I'm wrong. > Best regards, > Tomasz > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
Tomasz,
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Tomasz Figa
> Sent: Tuesday, August 8, 2017 7:43 AM
> To: Marathe, Yogesh
> On Mon, Aug 7, 2017 at 2:19 PM, Marathe, Yogesh
> wrote:
> > Tomasz,
> >
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> Sent: Saturday, August 5, 2017 8:47 AM
> >>
> >> Hi Yogesh,
> >>
> >> On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh
> >>
> >> wrote:
> >> >> -Original Message-
> >> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> >> Sent: Friday, August 4, 2017 9:39 PM On Sat, Aug 5, 2017 at 12:53
> >> >> AM, Marathe, Yogesh wrote:
> >> >> > Tomasz, Emil,
> >> >> >
> >> >> >> -Original Message-
> >> >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4,
> >> >> >> 2017 at 9:55 PM, Emil Velikov
> >> >> wrote:
> >> >> >> >>> >> - version check (2+) the fence extension, calling
> >> >> >> >>> >> .create_fence_fd() only when
> >> >> >> >>> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
> >> >> >> >>
> >> >> >> >> The check looks like below now, this is in
> >> >> >> >> dri2_surf_update_fence_fd() before
> >> >> >> create_fence_fd is called.
> >> >> >> >>
> >> >> >> >> if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
> >> >> >> >>if(__DRI_FENCE_CAP_NATIVE_FD | dri2_dpy->fence-
> >> >> >> >get_capabilities(dri2_dpy->dri_screen)) {
> >> >> >> >> //create_fence_fd call
> >> >> >> >>}
> >> >> >> >> }
> >> >> >> >>
> >> >> >> > Close but no cigar.
> >> >> >> >
> >> >> >> > if (dri2_surf->enable_out_fence && dri2_dpy->fence &&
> >> >> >> > dri2_dpy->fence->base.version >= 2 &&
> >> >> >> > dri2_dpy->fence->get_capabilities) {
> >> >> >> >
> >> >> >> >if
> >> >> >> > (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
> >> >> >> > __DRI_FENCE_CAP_NATIVE_FD) {
> >> >> >> > //create_fence_fd call
> >> >> >> >}
> >> >> >> > }
> >> >> >>
> >> >> >> If this needs so complicated series of checks, maybe it would
> >> >> >> make more sense to just set enable_out_fence based on
> >> >> >> availability of the capability at initialization time?
> >> >> >
> >> >> > I liked this one compared to nested ifs in
> >> >> > dri2_surf_update_fence_fd().
> >> >> >
> >> >> >>
> >> >> >> >
> >> >> >> >> Overall, if I further go ahead and check, actually
> >> >> >> >> get_capabilities() ultimately returns based on
> >> >> >> >> has_exec_fence which depends on I915_PARAM_HAS_EXEC_FENCE.
> >> >> >> >> This is always set to true for i915 in kernel drv unless
> >> >> >> >> forced to false!! I'm not sure if that inner check of
> >> >> >> get_capabilities still makes sense. Isn't the first one sufficient?
> >> >> >> >>
> >> >> >> > Not sure what you mean with "first one", but consider the
> >> >> >> > following
> >> >> example:
> >> >> >> > - old kernel which does not support (or has force disabled)
> >> >> >> > I915_PARAM_HAS_EXEC_FENCE.
> >> >> >> > - new userspace which unconditionally advertises the fence
> >> >> >> > v2 extension IIRC one may tweak that things to only
> >> >> >> > conditionally advertise it, but IMHO it's not worth the hassle.
> >> >> >> >
> >> >> >> > Even then, Mesa can produce 20 DRI driv
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
> -Original Message-
>
> Tomasz,
>
> > -Original Message-
> > From: Tomasz Figa [mailto:tf...@chromium.org]
> > Sent: Saturday, August 5, 2017 8:47 AM
> >
> > Hi Yogesh,
> >
> > On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh
> >
> > wrote:
> > >> -Original Message-
> > >> From: Tomasz Figa [mailto:tf...@chromium.org]
> > >> Sent: Friday, August 4, 2017 9:39 PM On Sat, Aug 5, 2017 at 12:53
> > >> AM, Marathe, Yogesh wrote:
> > >> > Tomasz, Emil,
> > >> >
> > >> >> -Original Message-
> > >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4,
> > >> >> 2017 at 9:55 PM, Emil Velikov
> > >> wrote:
> > >> >> >>> >> - version check (2+) the fence extension, calling
> > >> >> >>> >> .create_fence_fd() only when
> > >> >> >>> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
> > >> >> >>
> > >> >> >> The check looks like below now, this is in
> > >> >> >> dri2_surf_update_fence_fd() before
> > >> >> create_fence_fd is called.
> > >> >> >>
> > >> >> >> if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
> > >> >> >>if(__DRI_FENCE_CAP_NATIVE_FD | dri2_dpy->fence-
> > >> >> >get_capabilities(dri2_dpy->dri_screen)) {
> > >> >> >> //create_fence_fd call
> > >> >> >>}
> > >> >> >> }
> > >> >> >>
> > >> >> > Close but no cigar.
> > >> >> >
> > >> >> > if (dri2_surf->enable_out_fence && dri2_dpy->fence &&
> > >> >> > dri2_dpy->fence->base.version >= 2 &&
> > >> >> > dri2_dpy->fence->get_capabilities) {
> > >> >> >
> > >> >> >if (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen)
> > >> >> > &
> > >> >> > __DRI_FENCE_CAP_NATIVE_FD) {
> > >> >> > //create_fence_fd call
> > >> >> >}
> > >> >> > }
> > >> >>
> > >> >> If this needs so complicated series of checks, maybe it would
> > >> >> make more sense to just set enable_out_fence based on
> > >> >> availability of the capability at initialization time?
> > >> >
> > >> > I liked this one compared to nested ifs in dri2_surf_update_fence_fd().
> > >> >
> > >> >>
> > >> >> >
> > >> >> >> Overall, if I further go ahead and check, actually
> > >> >> >> get_capabilities() ultimately returns based on has_exec_fence
> > >> >> >> which depends on I915_PARAM_HAS_EXEC_FENCE. This is always
> > >> >> >> set to true for i915 in kernel drv unless forced to false!!
> > >> >> >> I'm not sure if that inner check of
> > >> >> get_capabilities still makes sense. Isn't the first one sufficient?
> > >> >> >>
> > >> >> > Not sure what you mean with "first one", but consider the
> > >> >> > following
> > >> example:
> > >> >> > - old kernel which does not support (or has force disabled)
> > >> >> > I915_PARAM_HAS_EXEC_FENCE.
> > >> >> > - new userspace which unconditionally advertises the fence v2
> > >> >> > extension IIRC one may tweak that things to only conditionally
> > >> >> > advertise it, but IMHO it's not worth the hassle.
> > >> >> >
> > >> >> > Even then, Mesa can produce 20 DRI drivers (used by the EGL
> > >> >> > module) so focusing on one doesn't quite work.
> > >> >> >
> > >> >> >>> >> - don't introduce unused variables (in make_current)
> > >> >> >>
> > >> >> >> Done.
> > >> >> >>
> > >> >> >>> >> - the create fd for the old display surface (in
> > >> >> >>> >> make_current) seems bogus
> > >> >&
Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965: Return the last fence if the batch buffer is empty and nothing to be flushed when _intel_batchbuffer_flush_fence.
> -Original Message- > From: Wu, Zhongmin > Sent: Monday, August 7, 2017 11:27 AM > To: Marathe, Yogesh ; Tomasz Figa > > Cc: Gao, Shuo ; Antognolli, Rafael > ; Emil Velikov ; > Kenneth Graunke ; ML mesa-dev d...@lists.freedesktop.org>; Kondapally, Kalyan > ; Timothy Arceri > Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965: > Return the last fence if the batch buffer is empty and nothing to be flushed > when > _intel_batchbuffer_flush_fence. > > Hi Yogesh: > http://oorja.iind.intel.com/mediawiki/index.php/Flatland > can you also try the flatland on this page. > > For AOSP flatland, yes, the EGL patch may solve the issue. That should suffice. > > However, I met one case that the batch buffer is empty just at the > swapbuffer > (glfush is just called before that), then you might not get a fence fd. So I > had to > record the last fence fd. > > I am not sure, you had better try it on the board If you can get a valid fd > on any > situation. I think aosp flatland is more relevant here. I haven’t face the issue you have mentioned so far. > > > > > > -Original Message- > From: Marathe, Yogesh > Sent: Monday, August 7, 2017 13:25 > To: Tomasz Figa ; Wu, Zhongmin > > Cc: Gao, Shuo ; Antognolli, Rafael > ; Emil Velikov ; > Kenneth Graunke ; ML mesa-dev d...@lists.freedesktop.org>; Kondapally, Kalyan > ; Timothy Arceri > Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965: > Return the last fence if the batch buffer is empty and nothing to be flushed > when > _intel_batchbuffer_flush_fence. > > This can be dropped. I'm running with egl patch alone and things seem fine. > > Zhongmin, please comment if you don’t think so. > > > -Original Message- > > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > > Behalf Of Marathe, Yogesh > > Sent: Friday, August 4, 2017 9:18 PM > > > > Tomasz, > > > > > -Original Message- > > > From: Tomasz Figa [mailto:tf...@chromium.org] > > > Sent: Friday, August 4, 2017 3:48 PM > > > To: Marathe, Yogesh > > > Cc: Emil Velikov ; Wu, Zhongmin > > > ; Gao, Shuo ; Antognolli, > > > Rafael ; Timothy Arceri > > > ; Kenneth Graunke ; > > > Kondapally, Kalyan ; ML mesa-dev > > d...@lists.freedesktop.org> > > > Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation > > > 1/2] > > i965: > > > Return the last fence if the batch buffer is empty and nothing to be > > > flushed when _intel_batchbuffer_flush_fence. > > > > > > Hi Yogesh, > > > > > > On Fri, Aug 4, 2017 at 7:12 PM, Marathe, Yogesh > > > > > > wrote: > > > > Hi Emil, > > > > > > > >> -Original Message- > > > >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > > > >> Behalf Of Emil Velikov > > > >> Sent: Tuesday, July 25, 2017 8:19 PM > > > >> To: Wu, Zhongmin > > > >> Cc: Gao, Shuo ; Antognolli, Rafael > > > >> ; Timothy Arceri > > > >> ; Marathe, Yogesh > > > >> ; Tomasz Figa ; > > > >> Kenneth Graunke ; Kondapally, Kalyan > > > >> ; ML mesa-dev > > >> d...@lists.freedesktop.org> > > > >> Subject: Re: [Mesa-dev] [EGL android: accquire fence > > > >> implementation 1/2] > > > i965: > > > >> Return the last fence if the batch buffer is empty and nothing to > > > >> be flushed when _intel_batchbuffer_flush_fence. > > > >> > > > >> Hi Zhongmin, > > > >> > > > >> Is the issue resolved by the EGL patch alone? Worth sticking with > > > >> that for > > > now? > > > >> > > > >> I think this patch will cause some noticeable overhead - see > > > >> below for > > details. > > > >> > > > >> > > > >> On 21 July 2017 at 04:08, Zhongmin Wu > wrote: > > > >> > Always save the last fence in the brw context when flushing buffer. > > > >> > If the buffer is nothing to be flushed, then return the last > > > >> > fence when asked for. > > > >> > > > > >> > Change-Id: Ic47035bcd1a27e402609afd9e2d1e3972548b97d > > > >> > Signed-off-by: Zhongmin Wu > > > >> > --- > > > >> > src/mesa/drivers/dri/i965/brw_co
Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965: Return the last fence if the batch buffer is empty and nothing to be flushed when _intel_batchbuffer_flush_fence.
This can be dropped. I'm running with egl patch alone and things seem fine.
Zhongmin, please comment if you don’t think so.
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Marathe, Yogesh
> Sent: Friday, August 4, 2017 9:18 PM
>
> Tomasz,
>
> > -Original Message-
> > From: Tomasz Figa [mailto:tf...@chromium.org]
> > Sent: Friday, August 4, 2017 3:48 PM
> > To: Marathe, Yogesh
> > Cc: Emil Velikov ; Wu, Zhongmin
> > ; Gao, Shuo ; Antognolli,
> > Rafael ; Timothy Arceri
> > ; Kenneth Graunke ;
> > Kondapally, Kalyan ; ML mesa-dev > d...@lists.freedesktop.org>
> > Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2]
> i965:
> > Return the last fence if the batch buffer is empty and nothing to be
> > flushed when _intel_batchbuffer_flush_fence.
> >
> > Hi Yogesh,
> >
> > On Fri, Aug 4, 2017 at 7:12 PM, Marathe, Yogesh
> >
> > wrote:
> > > Hi Emil,
> > >
> > >> -Original Message-
> > >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> > >> Behalf Of Emil Velikov
> > >> Sent: Tuesday, July 25, 2017 8:19 PM
> > >> To: Wu, Zhongmin
> > >> Cc: Gao, Shuo ; Antognolli, Rafael
> > >> ; Timothy Arceri
> > >> ; Marathe, Yogesh
> > >> ; Tomasz Figa ;
> > >> Kenneth Graunke ; Kondapally, Kalyan
> > >> ; ML mesa-dev > >> d...@lists.freedesktop.org>
> > >> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation
> > >> 1/2]
> > i965:
> > >> Return the last fence if the batch buffer is empty and nothing to
> > >> be flushed when _intel_batchbuffer_flush_fence.
> > >>
> > >> Hi Zhongmin,
> > >>
> > >> Is the issue resolved by the EGL patch alone? Worth sticking with
> > >> that for
> > now?
> > >>
> > >> I think this patch will cause some noticeable overhead - see below for
> details.
> > >>
> > >>
> > >> On 21 July 2017 at 04:08, Zhongmin Wu wrote:
> > >> > Always save the last fence in the brw context when flushing buffer.
> > >> > If the buffer is nothing to be flushed, then return the last
> > >> > fence when asked for.
> > >> >
> > >> > Change-Id: Ic47035bcd1a27e402609afd9e2d1e3972548b97d
> > >> > Signed-off-by: Zhongmin Wu
> > >> > ---
> > >> > src/mesa/drivers/dri/i965/brw_context.c |5 +
> > >> > src/mesa/drivers/dri/i965/brw_context.h |1 +
> > >> > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 16 ++--
> > >> > 3 files changed, 20 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> > >> > b/src/mesa/drivers/dri/i965/brw_context.c
> > >> > index 5433f90..ed0b056 100644
> > >> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > >> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > >> > @@ -1086,6 +1086,8 @@ brwCreateContext(gl_api api,
> > >> > ctx->VertexProgram._MaintainTnlProgram = true;
> > >> > ctx->FragmentProgram._MaintainTexEnvProgram = true;
> > >> >
> > >> > + brw->out_fence_fd = -1;
> > >> > +
> > >> > brw_draw_init( brw );
> > >> >
> > >> > if ((flags & __DRI_CTX_FLAG_DEBUG) != 0) { @@ -1169,6 +1171,9
> > >> > @@ intelDestroyContext(__DRIcontext * driContextPriv)
> > >> > brw->throttle_batch[1] = NULL;
> > >> > brw->throttle_batch[0] = NULL;
> > >> >
> > >> > + if (brw->out_fence_fd >= 0)
> > >> > + close(brw->out_fence_fd);
> > >> > +
> > >> > driDestroyOptionCache(&brw->optionCache);
> > >> >
> > >> > /* free the Mesa context */
> > >> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> > >> > b/src/mesa/drivers/dri/i965/brw_context.h
> > >> > index dc4bc8f..692ea2c 100644
> > >> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > >> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > >> > @@ -1217,6 +1217,7 @@ struct brw_context
> > >> >
> > >> > __DRIcontext *driCo
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
Tomasz,
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Saturday, August 5, 2017 8:47 AM
>
> Hi Yogesh,
>
> On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh
> wrote:
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> Sent: Friday, August 4, 2017 9:39 PM
> >> On Sat, Aug 5, 2017 at 12:53 AM, Marathe, Yogesh
> >> wrote:
> >> > Tomasz, Emil,
> >> >
> >> >> -Original Message-
> >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4, 2017
> >> >> at 9:55 PM, Emil Velikov
> >> wrote:
> >> >> >>> >> - version check (2+) the fence extension, calling
> >> >> >>> >> .create_fence_fd() only when
> >> >> >>> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
> >> >> >>
> >> >> >> The check looks like below now, this is in
> >> >> >> dri2_surf_update_fence_fd() before
> >> >> create_fence_fd is called.
> >> >> >>
> >> >> >> if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
> >> >> >>if(__DRI_FENCE_CAP_NATIVE_FD | dri2_dpy->fence-
> >> >> >get_capabilities(dri2_dpy->dri_screen)) {
> >> >> >> //create_fence_fd call
> >> >> >>}
> >> >> >> }
> >> >> >>
> >> >> > Close but no cigar.
> >> >> >
> >> >> > if (dri2_surf->enable_out_fence && dri2_dpy->fence &&
> >> >> > dri2_dpy->fence->base.version >= 2 &&
> >> >> > dri2_dpy->fence->get_capabilities) {
> >> >> >
> >> >> >if (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
> >> >> > __DRI_FENCE_CAP_NATIVE_FD) {
> >> >> > //create_fence_fd call
> >> >> >}
> >> >> > }
> >> >>
> >> >> If this needs so complicated series of checks, maybe it would make
> >> >> more sense to just set enable_out_fence based on availability of
> >> >> the capability at initialization time?
> >> >
> >> > I liked this one compared to nested ifs in dri2_surf_update_fence_fd().
> >> >
> >> >>
> >> >> >
> >> >> >> Overall, if I further go ahead and check, actually
> >> >> >> get_capabilities() ultimately returns based on has_exec_fence
> >> >> >> which depends on I915_PARAM_HAS_EXEC_FENCE. This is always set
> >> >> >> to true for i915 in kernel drv unless forced to false!! I'm not
> >> >> >> sure if that inner check of
> >> >> get_capabilities still makes sense. Isn't the first one sufficient?
> >> >> >>
> >> >> > Not sure what you mean with "first one", but consider the
> >> >> > following
> >> example:
> >> >> > - old kernel which does not support (or has force disabled)
> >> >> > I915_PARAM_HAS_EXEC_FENCE.
> >> >> > - new userspace which unconditionally advertises the fence v2
> >> >> > extension IIRC one may tweak that things to only conditionally
> >> >> > advertise it, but IMHO it's not worth the hassle.
> >> >> >
> >> >> > Even then, Mesa can produce 20 DRI drivers (used by the EGL
> >> >> > module) so focusing on one doesn't quite work.
> >> >> >
> >> >> >>> >> - don't introduce unused variables (in make_current)
> >> >> >>
> >> >> >> Done.
> >> >> >>
> >> >> >>> >> - the create fd for the old display surface (in
> >> >> >>> >> make_current) seems bogus
> >> >> >>
> >> >> >> Done.
> >> >> >>
> >> >> > Did you drop it all together or changed to use some other surface?
> >> >> > Would be nice to hear the reason why it was added - perhaps I'm
> >> >> > missing something.
> >> >>
> >> >> We have to keep it, otherwise there would be no fence available at
> >> >> th
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, August 4, 2017 9:39 PM
> On Sat, Aug 5, 2017 at 12:53 AM, Marathe, Yogesh
> wrote:
> > Tomasz, Emil,
> >
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> On Fri, Aug 4, 2017 at 9:55 PM, Emil Velikov
> wrote:
> >> >>> >> - version check (2+) the fence extension, calling
> >> >>> >> .create_fence_fd() only when
> >> >>> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
> >> >>
> >> >> The check looks like below now, this is in
> >> >> dri2_surf_update_fence_fd() before
> >> create_fence_fd is called.
> >> >>
> >> >> if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
> >> >>if(__DRI_FENCE_CAP_NATIVE_FD | dri2_dpy->fence-
> >> >get_capabilities(dri2_dpy->dri_screen)) {
> >> >> //create_fence_fd call
> >> >>}
> >> >> }
> >> >>
> >> > Close but no cigar.
> >> >
> >> > if (dri2_surf->enable_out_fence && dri2_dpy->fence &&
> >> > dri2_dpy->fence->base.version >= 2 &&
> >> > dri2_dpy->fence->get_capabilities) {
> >> >
> >> >if (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
> >> > __DRI_FENCE_CAP_NATIVE_FD) {
> >> > //create_fence_fd call
> >> >}
> >> > }
> >>
> >> If this needs so complicated series of checks, maybe it would make
> >> more sense to just set enable_out_fence based on availability of the
> >> capability at initialization time?
> >
> > I liked this one compared to nested ifs in dri2_surf_update_fence_fd().
> >
> >>
> >> >
> >> >> Overall, if I further go ahead and check, actually
> >> >> get_capabilities() ultimately returns based on has_exec_fence
> >> >> which depends on I915_PARAM_HAS_EXEC_FENCE. This is always set to
> >> >> true for i915 in kernel drv unless forced to false!! I'm not sure
> >> >> if that inner check of
> >> get_capabilities still makes sense. Isn't the first one sufficient?
> >> >>
> >> > Not sure what you mean with "first one", but consider the following
> example:
> >> > - old kernel which does not support (or has force disabled)
> >> > I915_PARAM_HAS_EXEC_FENCE.
> >> > - new userspace which unconditionally advertises the fence v2
> >> > extension IIRC one may tweak that things to only conditionally
> >> > advertise it, but IMHO it's not worth the hassle.
> >> >
> >> > Even then, Mesa can produce 20 DRI drivers (used by the EGL module)
> >> > so focusing on one doesn't quite work.
> >> >
> >> >>> >> - don't introduce unused variables (in make_current)
> >> >>
> >> >> Done.
> >> >>
> >> >>> >> - the create fd for the old display surface (in make_current)
> >> >>> >> seems bogus
> >> >>
> >> >> Done.
> >> >>
> >> > Did you drop it all together or changed to use some other surface?
> >> > Would be nice to hear the reason why it was added - perhaps I'm
> >> > missing something.
> >>
> >> We have to keep it, otherwise there would be no fence available at
> >> the time of surface destruction, while, at least for Android, a fence
> >> can be passed to window's cancelBuffer callback.
> >>
> >> >
> >> > I think that we want a fence/fd for the new draw surface. Since
> >> > otherwise one won't get created up until the first SwapBuffers call.
> >>
> >> I might be missing something, but wouldn't that insert a fence at the
> >> beginning of command stream, before even doing anything? At least in
> >> Android use cases, the only places we need the fence is in
> >> SwapBuffers and DestroySurface and the fence should be inserted after
> >> all the commands for rendering into given surface.
> >>
> >
> > Emil,
> >
> > Tomasz sounds convincing to me here, I just went ahead with the
> > comment to try out and flatland worked even after removing that.
> > Zhongmin can expla
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
Tomasz, Emil,
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, August 4, 2017 6:54 PM
> To: Emil Velikov
> Cc: Marathe, Yogesh ; Antognolli, Rafael
> ; ML mesa-dev d...@lists.freedesktop.org>; Wu, Zhongmin ; Gao,
> Shuo ; Liu, Zhiquan ; Daniel
> Stone ; Timothy Arceri ; Eric
> Engestrom ; Kenneth Graunke ;
> Kondapally, Kalyan ; Varad Gautam
> ; Rainer Hochecker ;
> Nicolai Hähnle
> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync
> fence
> for Android OS
>
> On Fri, Aug 4, 2017 at 9:55 PM, Emil Velikov wrote:
> >>> >> - version check (2+) the fence extension, calling
> >>> >> .create_fence_fd() only when
> >>> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
> >>
> >> The check looks like below now, this is in dri2_surf_update_fence_fd()
> >> before
> create_fence_fd is called.
> >>
> >> if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
> >>if(__DRI_FENCE_CAP_NATIVE_FD | dri2_dpy->fence-
> >get_capabilities(dri2_dpy->dri_screen)) {
> >> //create_fence_fd call
> >>}
> >> }
> >>
> > Close but no cigar.
> >
> > if (dri2_surf->enable_out_fence && dri2_dpy->fence &&
> > dri2_dpy->fence->base.version >= 2 &&
> > dri2_dpy->fence->get_capabilities) {
> >
> >if (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
> > __DRI_FENCE_CAP_NATIVE_FD) {
> > //create_fence_fd call
> >}
> > }
>
> If this needs so complicated series of checks, maybe it would make more sense
> to just set enable_out_fence based on availability of the capability at
> initialization time?
I liked this one compared to nested ifs in dri2_surf_update_fence_fd().
>
> >
> >> Overall, if I further go ahead and check, actually get_capabilities()
> >> ultimately returns based on has_exec_fence which depends on
> >> I915_PARAM_HAS_EXEC_FENCE. This is always set to true for i915 in
> >> kernel drv unless forced to false!! I'm not sure if that inner check of
> get_capabilities still makes sense. Isn't the first one sufficient?
> >>
> > Not sure what you mean with "first one", but consider the following example:
> > - old kernel which does not support (or has force disabled)
> > I915_PARAM_HAS_EXEC_FENCE.
> > - new userspace which unconditionally advertises the fence v2
> > extension IIRC one may tweak that things to only conditionally
> > advertise it, but IMHO it's not worth the hassle.
> >
> > Even then, Mesa can produce 20 DRI drivers (used by the EGL module) so
> > focusing on one doesn't quite work.
> >
> >>> >> - don't introduce unused variables (in make_current)
> >>
> >> Done.
> >>
> >>> >> - the create fd for the old display surface (in make_current)
> >>> >> seems bogus
> >>
> >> Done.
> >>
> > Did you drop it all together or changed to use some other surface?
> > Would be nice to hear the reason why it was added - perhaps I'm
> > missing something.
>
> We have to keep it, otherwise there would be no fence available at the time of
> surface destruction, while, at least for Android, a fence can be passed to
> window's cancelBuffer callback.
>
> >
> > I think that we want a fence/fd for the new draw surface. Since
> > otherwise one won't get created up until the first SwapBuffers call.
>
> I might be missing something, but wouldn't that insert a fence at the
> beginning
> of command stream, before even doing anything? At least in Android use cases,
> the only places we need the fence is in SwapBuffers and DestroySurface and the
> fence should be inserted after all the commands for rendering into given
> surface.
>
Emil,
Tomasz sounds convincing to me here, I just went ahead with the comment to try
out and
flatland worked even after removing that. Zhongmin can explain better but I
think in earlier
revisions this was done for cancelBuffer to match with queueBuffer, I mean we
are passing
valid fd for queueBuffer by doing this we would have a valid fd during
cancelBuffer. Not
sure if this is the reason / one of the reason.
I will go ahead with rest of your comments if we are ok to keep fd for old
display surface
in make_current.
> Best regards,
> Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965: Return the last fence if the batch buffer is empty and nothing to be flushed when _intel_batchbuffer_flush_fence.
Tomasz,
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, August 4, 2017 3:48 PM
> To: Marathe, Yogesh
> Cc: Emil Velikov ; Wu, Zhongmin
> ; Gao, Shuo ; Antognolli,
> Rafael ; Timothy Arceri
> ; Kenneth Graunke ;
> Kondapally, Kalyan ; ML mesa-dev d...@lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965:
> Return the last fence if the batch buffer is empty and nothing to be flushed
> when
> _intel_batchbuffer_flush_fence.
>
> Hi Yogesh,
>
> On Fri, Aug 4, 2017 at 7:12 PM, Marathe, Yogesh
> wrote:
> > Hi Emil,
> >
> >> -Original Message-
> >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> >> Behalf Of Emil Velikov
> >> Sent: Tuesday, July 25, 2017 8:19 PM
> >> To: Wu, Zhongmin
> >> Cc: Gao, Shuo ; Antognolli, Rafael
> >> ; Timothy Arceri
> >> ; Marathe, Yogesh ;
> >> Tomasz Figa ; Kenneth Graunke
> >> ; Kondapally, Kalyan
> >> ; ML mesa-dev >> d...@lists.freedesktop.org>
> >> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2]
> i965:
> >> Return the last fence if the batch buffer is empty and nothing to be
> >> flushed when _intel_batchbuffer_flush_fence.
> >>
> >> Hi Zhongmin,
> >>
> >> Is the issue resolved by the EGL patch alone? Worth sticking with that for
> now?
> >>
> >> I think this patch will cause some noticeable overhead - see below for
> >> details.
> >>
> >>
> >> On 21 July 2017 at 04:08, Zhongmin Wu wrote:
> >> > Always save the last fence in the brw context when flushing buffer.
> >> > If the buffer is nothing to be flushed, then return the last fence
> >> > when asked for.
> >> >
> >> > Change-Id: Ic47035bcd1a27e402609afd9e2d1e3972548b97d
> >> > Signed-off-by: Zhongmin Wu
> >> > ---
> >> > src/mesa/drivers/dri/i965/brw_context.c |5 +
> >> > src/mesa/drivers/dri/i965/brw_context.h |1 +
> >> > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 16 ++--
> >> > 3 files changed, 20 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> >> > b/src/mesa/drivers/dri/i965/brw_context.c
> >> > index 5433f90..ed0b056 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> >> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> >> > @@ -1086,6 +1086,8 @@ brwCreateContext(gl_api api,
> >> > ctx->VertexProgram._MaintainTnlProgram = true;
> >> > ctx->FragmentProgram._MaintainTexEnvProgram = true;
> >> >
> >> > + brw->out_fence_fd = -1;
> >> > +
> >> > brw_draw_init( brw );
> >> >
> >> > if ((flags & __DRI_CTX_FLAG_DEBUG) != 0) { @@ -1169,6 +1171,9
> >> > @@ intelDestroyContext(__DRIcontext * driContextPriv)
> >> > brw->throttle_batch[1] = NULL;
> >> > brw->throttle_batch[0] = NULL;
> >> >
> >> > + if (brw->out_fence_fd >= 0)
> >> > + close(brw->out_fence_fd);
> >> > +
> >> > driDestroyOptionCache(&brw->optionCache);
> >> >
> >> > /* free the Mesa context */
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> >> > b/src/mesa/drivers/dri/i965/brw_context.h
> >> > index dc4bc8f..692ea2c 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> >> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> >> > @@ -1217,6 +1217,7 @@ struct brw_context
> >> >
> >> > __DRIcontext *driContext;
> >> > struct intel_screen *screen;
> >> > + int out_fence_fd;
> >> > };
> >> >
> >> > /* brw_clear.c */
> >> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > index 62d2fe8..d342e5d 100644
> >> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > @@ -648,9 +648,18 @@ do_flush_locked(struct brw_context *brw, int
> >> in_fence_fd, int *out_fence_fd)
> >> > /* Add the batch itself to the end of the validation list */
> >> > add_exec_bo(batch, batch->bo);
>
Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965: Return the last fence if the batch buffer is empty and nothing to be flushed when _intel_batchbuffer_flush_fence.
Hi Emil,
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Emil Velikov
> Sent: Tuesday, July 25, 2017 8:19 PM
> To: Wu, Zhongmin
> Cc: Gao, Shuo ; Antognolli, Rafael
> ; Timothy Arceri ;
> Marathe, Yogesh ; Tomasz Figa
> ; Kenneth Graunke ;
> Kondapally, Kalyan ; ML mesa-dev d...@lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965:
> Return the last fence if the batch buffer is empty and nothing to be flushed
> when
> _intel_batchbuffer_flush_fence.
>
> Hi Zhongmin,
>
> Is the issue resolved by the EGL patch alone? Worth sticking with that for
> now?
>
> I think this patch will cause some noticeable overhead - see below for
> details.
>
>
> On 21 July 2017 at 04:08, Zhongmin Wu wrote:
> > Always save the last fence in the brw context when flushing buffer. If
> > the buffer is nothing to be flushed, then return the last fence when
> > asked for.
> >
> > Change-Id: Ic47035bcd1a27e402609afd9e2d1e3972548b97d
> > Signed-off-by: Zhongmin Wu
> > ---
> > src/mesa/drivers/dri/i965/brw_context.c |5 +
> > src/mesa/drivers/dri/i965/brw_context.h |1 +
> > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 16 ++--
> > 3 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> > b/src/mesa/drivers/dri/i965/brw_context.c
> > index 5433f90..ed0b056 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -1086,6 +1086,8 @@ brwCreateContext(gl_api api,
> > ctx->VertexProgram._MaintainTnlProgram = true;
> > ctx->FragmentProgram._MaintainTexEnvProgram = true;
> >
> > + brw->out_fence_fd = -1;
> > +
> > brw_draw_init( brw );
> >
> > if ((flags & __DRI_CTX_FLAG_DEBUG) != 0) { @@ -1169,6 +1171,9 @@
> > intelDestroyContext(__DRIcontext * driContextPriv)
> > brw->throttle_batch[1] = NULL;
> > brw->throttle_batch[0] = NULL;
> >
> > + if (brw->out_fence_fd >= 0)
> > + close(brw->out_fence_fd);
> > +
> > driDestroyOptionCache(&brw->optionCache);
> >
> > /* free the Mesa context */
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> > b/src/mesa/drivers/dri/i965/brw_context.h
> > index dc4bc8f..692ea2c 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > @@ -1217,6 +1217,7 @@ struct brw_context
> >
> > __DRIcontext *driContext;
> > struct intel_screen *screen;
> > + int out_fence_fd;
> > };
> >
> > /* brw_clear.c */
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index 62d2fe8..d342e5d 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -648,9 +648,18 @@ do_flush_locked(struct brw_context *brw, int
> in_fence_fd, int *out_fence_fd)
> > /* Add the batch itself to the end of the validation list */
> > add_exec_bo(batch, batch->bo);
> >
> > + if (brw->out_fence_fd >= 0) {
> > +close(brw->out_fence_fd);
> > +brw->out_fence_fd = -1;
> > + }
> > +
> > + int fd = -1;
> > ret = execbuffer(dri_screen->fd, batch, hw_ctx,
> >4 * USED_BATCH(*batch),
> > - in_fence_fd, out_fence_fd, flags);
> > + in_fence_fd, &fd, flags);
> execbuffer() creates an out fence if the "out_fence_fd" pointer is non-NULL.
> Hence with this patch we'will create a fence for each
> _intel_batchbuffer_flush_fence() invocation...
>
> Not sure how costly that will be though :-\
>
I see this results into 1 get_unused_fd_flags() + 1 sync_file_create() and
operations to
store out fd in kernel for return arg. I doubt it will be very costly, the ioctl
DRM_IOCTL_I915_GEM_EXECBUFFER2 or DRM_IOCTL_I915_GEM_EXECBUFFER2_WR
was anyways there, so nothing huge additional on ioctl front.
Nonetheless, what other option do we have? This may sound absurd but is there a
way / ioctl to call sync_file_create directly from mesa UMD and store fds
instead of
creating at every execbuffer()? We also need a way to associate those stored
fds to
buffers then. I know I'm adding more ioctls but intension is, if we can do this
in some
init() we'll save these operations during execbuffer().
IMHO, this is separate discussion, let this patch enable functionality first
and then
we can work on making it light.
> -Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
Tomasz, > -Original Message- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Friday, August 4, 2017 6:52 AM > To: Marathe, Yogesh > Cc: Emil Velikov ; Gao, Shuo > ; Liu, Zhiquan ; Daniel Stone > ; Nicolai Hähnle ; > Antognolli, Rafael ; Eric Engestrom > ; Wu, Zhongmin ; Kenneth > Graunke ; Kondapally, Kalyan > ; Rainer Hochecker ; > ML mesa-dev ; Timothy Arceri > ; Varad Gautam > Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync > fence > for Android OS > > Hi Yogesh, > > On Fri, Aug 4, 2017 at 1:55 AM, Marathe, Yogesh > wrote: > > Adding folks who were CCed for earlier versions. > > > > Hi Emil, few doubts and comments below. > > > >> -Original Message- > >> From: Tomasz Figa [mailto:tf...@chromium.org] > >> Sent: Thursday, August 3, 2017 7:19 PM > >> To: Marathe, Yogesh > >> Cc: Emil Velikov ; Antognolli, Rafael > >> ; ML mesa-dev >> d...@lists.freedesktop.org>; Wu, Zhongmin > >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a > >> sync fence for Android OS > >> > >> On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh > >> wrote: > >> > Emil, > >> > > >> >> -Original Message- > >> >> From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > >> >> Sent: Thursday, August 3, 2017 4:06 PM > >> >> To: Marathe, Yogesh > >> >> Cc: ML mesa-dev ; Wu, Zhongmin > >> >> > >> >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with > >> >> a sync fence for Android OS > >> >> > >> >> On 2 August 2017 at 20:01, wrote: > >> >> > From: Zhongmin Wu > >> >> > > >> >> > Before we queued the buffer with a invalid fence (-1), it causes > >> >> > benchmarks such as flatland to fail. This patch enables explicit > >> >> > sync feature on android. > >> >> > > >> >> > Now we get the out fence during the flushing buffer and then > >> >> > pass it to SurfaceFlinger in eglSwapbuffer function. > >> >> > > >> >> > v2: a) Also implement the fence in cancelBuffer. > >> >> > b) The last sync fence is stored in drawable object > >> >> >rather than brw context. > >> >> > c) format clear. > >> >> > > >> >> > v3: a) Save the last fence fd in DRI Context object. > >> >> > b) Return the last fence if the batch buffer is empty and > >> >> >nothing to be flushed when _intel_batchbuffer_flush_fence > >> >> > c) Add the new interface in vbtl to set the retrieve fence > >> >> > > >> >> > v3.1 a) close fd in the new vbtl interface on none Android > >> >> > platform > >> >> > > >> >> > v4: a) The last fence is saved in brw context. > >> >> > b) The retrieve fd is for all the platform but not just Android > >> >> > c) Add a uniform dri2 interface to initialize the surface. > >> >> > > >> >> > v4.1: a) make some changes of variable name. > >> >> > b) the patch is broken into two patches. > >> >> > > >> >> > v4.2: a) Add a deinit interface for surface to clear the out > >> >> > fence > >> >> > > >> >> > v5: a) Add enable_out_fence to init, platform sets it true or > >> >> >false > >> >> > b) Change get fd to update fd and check for fence > >> >> > c) Commit description updated > >> >> > > >> >> Seems like most suggestions in last version did not get addressed. > >> >> Please comment if you disagree (it can be that we've > >> >> misread/misremember > >> >> something) before posting another version. > >> >> > >> >> To reiterate: > >> >> - add blank line between variable declarations and code > > > > Done. > > > >> >> - use more consistent function names > > > > Do you want any specific functioned to be renamed? > > > >> >> - comment above queueBuffer needs fixing > > > > It reads as below now. > >/* Queue the buffer with stored out fence fd. The ANativeWindow or buffer > >
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
Adding folks who were CCed for earlier versions.
Hi Emil, few doubts and comments below.
> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Thursday, August 3, 2017 7:19 PM
> To: Marathe, Yogesh
> Cc: Emil Velikov ; Antognolli, Rafael
> ; ML mesa-dev d...@lists.freedesktop.org>; Wu, Zhongmin
> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync
> fence
> for Android OS
>
> On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh
> wrote:
> > Emil,
> >
> >> -Original Message-
> >> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> >> Sent: Thursday, August 3, 2017 4:06 PM
> >> To: Marathe, Yogesh
> >> Cc: ML mesa-dev ; Wu, Zhongmin
> >>
> >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a
> >> sync fence for Android OS
> >>
> >> On 2 August 2017 at 20:01, wrote:
> >> > From: Zhongmin Wu
> >> >
> >> > Before we queued the buffer with a invalid fence (-1), it causes
> >> > benchmarks such as flatland to fail. This patch enables explicit
> >> > sync feature on android.
> >> >
> >> > Now we get the out fence during the flushing buffer and then pass
> >> > it to SurfaceFlinger in eglSwapbuffer function.
> >> >
> >> > v2: a) Also implement the fence in cancelBuffer.
> >> > b) The last sync fence is stored in drawable object
> >> >rather than brw context.
> >> > c) format clear.
> >> >
> >> > v3: a) Save the last fence fd in DRI Context object.
> >> > b) Return the last fence if the batch buffer is empty and
> >> >nothing to be flushed when _intel_batchbuffer_flush_fence
> >> > c) Add the new interface in vbtl to set the retrieve fence
> >> >
> >> > v3.1 a) close fd in the new vbtl interface on none Android platform
> >> >
> >> > v4: a) The last fence is saved in brw context.
> >> > b) The retrieve fd is for all the platform but not just Android
> >> > c) Add a uniform dri2 interface to initialize the surface.
> >> >
> >> > v4.1: a) make some changes of variable name.
> >> > b) the patch is broken into two patches.
> >> >
> >> > v4.2: a) Add a deinit interface for surface to clear the out fence
> >> >
> >> > v5: a) Add enable_out_fence to init, platform sets it true or
> >> >false
> >> > b) Change get fd to update fd and check for fence
> >> > c) Commit description updated
> >> >
> >> Seems like most suggestions in last version did not get addressed.
> >> Please comment if you disagree (it can be that we've
> >> misread/misremember
> >> something) before posting another version.
> >>
> >> To reiterate:
> >> - add blank line between variable declarations and code
Done.
> >> - use more consistent function names
Do you want any specific functioned to be renamed?
> >> - comment above queueBuffer needs fixing
It reads as below now.
/* Queue the buffer with stored out fence fd. The ANativeWindow or buffer
* consumer may choose to wait for the fence to signal before accessing
* it. If fence fd value is -1, buffer can be accessed by consumer
* immediately. Consumer or application shouldn't rely on timestamp
* associated with fence if the fence fd is -1.
*
* Ownership of fd is transferred to consumer after queueBuffer and the
* consumer is responsible for closing it. Caller must not use the fd
* after passing it to queueBuffer.
*/
Looks ok?
> >> - version check (2+) the fence extension, calling .create_fence_fd()
> >> only when
> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
The check looks like below now, this is in dri2_surf_update_fence_fd() before
create_fence_fd is called.
if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
if(__DRI_FENCE_CAP_NATIVE_FD |
dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen)) {
//create_fence_fd call
}
}
Overall, if I further go ahead and check, actually get_capabilities()
ultimately returns based on
has_exec_fence which depends on I915_PARAM_HAS_EXEC_FENCE. This is always set
to
true for i915 in kernel drv unless forced to false!! I'm not sure if that inner
check of get_capabilities
still makes sense. Isn't the first one sufficient?
> >> - don't introduce unused variables (in make_current)
Done.
Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS
Emil,
> -Original Message-
> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> Sent: Thursday, August 3, 2017 4:06 PM
> To: Marathe, Yogesh
> Cc: ML mesa-dev ; Wu, Zhongmin
>
> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync
> fence
> for Android OS
>
> On 2 August 2017 at 20:01, wrote:
> > From: Zhongmin Wu
> >
> > Before we queued the buffer with a invalid fence (-1), it causes
> > benchmarks such as flatland to fail. This patch enables explicit sync
> > feature on android.
> >
> > Now we get the out fence during the flushing buffer and then pass it
> > to SurfaceFlinger in eglSwapbuffer function.
> >
> > v2: a) Also implement the fence in cancelBuffer.
> > b) The last sync fence is stored in drawable object
> >rather than brw context.
> > c) format clear.
> >
> > v3: a) Save the last fence fd in DRI Context object.
> > b) Return the last fence if the batch buffer is empty and
> >nothing to be flushed when _intel_batchbuffer_flush_fence
> > c) Add the new interface in vbtl to set the retrieve fence
> >
> > v3.1 a) close fd in the new vbtl interface on none Android platform
> >
> > v4: a) The last fence is saved in brw context.
> > b) The retrieve fd is for all the platform but not just Android
> > c) Add a uniform dri2 interface to initialize the surface.
> >
> > v4.1: a) make some changes of variable name.
> > b) the patch is broken into two patches.
> >
> > v4.2: a) Add a deinit interface for surface to clear the out fence
> >
> > v5: a) Add enable_out_fence to init, platform sets it true or
> >false
> > b) Change get fd to update fd and check for fence
> > c) Commit description updated
> >
> Seems like most suggestions in last version did not get addressed.
> Please comment if you disagree (it can be that we've misread/misremember
> something) before posting another version.
>
> To reiterate:
> - add blank line between variable declarations and code
> - use more consistent function names
> - comment above queueBuffer needs fixing
> - version check (2+) the fence extension, calling .create_fence_fd() only
> when
> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
> - don't introduce unused variables (in make_current)
> - the create fd for the old display surface (in make_current) seems bogus
>
I think the patch has gone through headline changes, so it's bit difficult to
track back. I tried
to address Rafael's comments (latest before v5) assuming others were taken care
of, after
4 versions. Doesn’t seem to be the case. No problem, thanks for re-iterating.
I'll work on
fixing these and discuss as required.
> Also, please change the commit summary message to reflect reality.
> I'm thinking of the following, although you can come with something better.
>
> "egl: allow creation of per surface out fence
>
> Add plumbing to allow creation of per display surface out fence.
>
> Currently enabled only on Android, since the system expects a valid fd in
> ANativeWindow::{queue,cancel}Buffer.
> Without it tools such as flatland will fail, since they cannot get the
> timestamp as
> we currently pass an fd of -1."
>
Ok. This sounds good to me, will also check commit message while pushing next
version.
> Thanks
> Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Avoids loop for buffer object availability in add_exec_bo
> > -Original Message- > > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] > > Sent: Friday, July 28, 2017 2:30 PM > > To: Muthukumar, Aravindan ; mesa- > > d...@lists.freedesktop.org > > Cc: Marathe, Yogesh ; Muthukumar, Aravindan > > > > Subject: Re: [Mesa-dev] [PATCH] i965: Avoids loop for buffer object > > availability in add_exec_bo > > > > Quoting aravindan.muthuku...@intel.com (2017-07-28 09:37:01) > > > From: Aravindan Muthukumar > > > > > > Original logic loops over the list for every buffer object. > > > Maintained a flag to identify whether bo is already there in list. > > > > No. brw_bo is shared between many contexts, and so you are marking it > > as available in every one. May I suggest looking for issues in > > https://patchwork.freedesktop.org/series/27719/ and help bring that > > series to fruition. > > Thanks Chris, we'll get back. Agreed. The patch series you have pointed out covers it, patch can be dropped. We tried applying series on master but it didn't apply cleanly, is there a base commit/branch on which this was tested / expected to work? > > > -Chris > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence for Android OS v4.2
Rafael, Tomasz,
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Rafael Antognolli
> Sent: Tuesday, July 25, 2017 9:39 PM
> To: Wu, Zhongmin
> Cc: Gao, Shuo ; Liu, Zhiquan ;
> Daniel Stone ; emil.l.veli...@gmail.com; Eric
> Engestrom ; Marathe, Yogesh
> ; tf...@chromium.org; Kondapally, Kalyan
> ; mesa-dev@lists.freedesktop.org; Varad
> Gautam
> Subject: Re: [Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence
> for Android OS v4.2
>
> On Tue, Jul 25, 2017 at 10:07:23AM +0800, Zhongmin Wu wrote:
> > Before we queued the buffer with a invalid fence (-1), it will make
> > some benchmarks failed to test such as flatland.
> >
> > Now we get the out fence during the flushing buffer and then pass it
> > to SurfaceFlinger in eglSwapbuffer function.
> >
> > v2: a) Also implement the fence in cancelBuffer.
> > b) The last sync fence is stored in drawable object
> >rather than brw context.
> > c) format clear.
> >
> > v3: a) Save the last fence fd in DRI Context object.
> > b) Return the last fence if the batch buffer is empty and
> >nothing to be flushed when _intel_batchbuffer_flush_fence
> > c) Add the new interface in vbtl to set the retrieve fence
> >
> > v3.1 a) close fd in the new vbtl interface on none Android platform
> >
> > v4: a) The last fence is saved in brw context.
> > b) The retrieve fd is for all the platform but not just Android
> > c) Add a uniform dri2 interface to initialize the surface.
> >
> > v4.1: a) make some changes of variable name.
> > b) the patch is breaked into two patches.
> >
> > v4.2: a) Add a deinit interface for surface to clear the out fence
>
> Hi Zhongmin,
>
> The patch is indeed looking better. I agree with Tomasz, it would be good to
> have a way for the platform to inform whether it is interested in fences or
> not.
> What about some flag that you pass to dri2_surf_init? (I'm not sure that's the
> best place, though).
>
I would like to take it forward from here for remaining review comments,
Zhongmin agrees.
I added 'enable_out_fence' bool to dri2_surf_init() as a parameter and all
platforms except
android pass this as false. Based on 'enable_out_fence' stored with surface,
'dri2_surf_get/update_fence_fd()' has a check before it calls
create_fence_fd(). Is this the
right expectation here?
Please let me know if 'enable_out_fence' can be changed for a better name. I've
already
implemented other review other comments Rafael mentioned and will include them
in v4.3 along with this.
> Please see other comments below.
>
> > Change-Id: Ided54d2e193cde73a6f0feb36ac1c0056e4958f2
> > Signed-off-by: Zhongmin Wu
> > ---
> > src/egl/drivers/dri2/egl_dri2.c | 51
> > +++
> > src/egl/drivers/dri2/egl_dri2.h |8 +
> > src/egl/drivers/dri2/platform_android.c | 12 ---
> > src/egl/drivers/dri2/platform_drm.c |3 +-
> > src/egl/drivers/dri2/platform_surfaceless.c |3 +-
> > src/egl/drivers/dri2/platform_wayland.c |3 +-
> > src/egl/drivers/dri2/platform_x11.c |3 +-
> > src/egl/drivers/dri2/platform_x11_dri3.c|3 +-
> > 8 files changed, 77 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/egl_dri2.c
> > b/src/egl/drivers/dri2/egl_dri2.c index 020a0bc..ffd3a8a 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.c
> > +++ b/src/egl/drivers/dri2/egl_dri2.c
> > @@ -1307,6 +1307,32 @@ dri2_destroy_context(_EGLDriver *drv,
> _EGLDisplay *disp, _EGLContext *ctx)
> > return EGL_TRUE;
> > }
> >
> > +EGLBoolean
> > +dri2_surf_init(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> > +_EGLConfig *conf, const EGLint *attrib_list) {
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + dri2_surf->out_fence_fd = -1;
> > + return _eglInitSurface(surf, dpy, type, conf, attrib_list); }
> > +
> > +static void
> > +dri2_surface_set_out_fence( _EGLSurface *surf, int fence_fd) {
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + if (dri2_surf->out_fence_fd >=0)
> > + close(dri2_surf->out_fence_fd);
> > +
> > + dri2_surf->out_fence_fd = fence_fd; }
> > +
> > +void
> > +dri2_surf_deinit(_EGLSurface *surf)
> > +{
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + dri2_surface_set_o
Re: [Mesa-dev] [PATCH] i965: Avoids loop for buffer object availability in add_exec_bo
> -Original Message- > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] > Sent: Friday, July 28, 2017 2:30 PM > To: Muthukumar, Aravindan ; mesa- > d...@lists.freedesktop.org > Cc: Marathe, Yogesh ; Muthukumar, Aravindan > > Subject: Re: [Mesa-dev] [PATCH] i965: Avoids loop for buffer object > availability > in add_exec_bo > > Quoting aravindan.muthuku...@intel.com (2017-07-28 09:37:01) > > From: Aravindan Muthukumar > > > > Original logic loops over the list for every buffer object. Maintained > > a flag to identify whether bo is already there in list. > > No. brw_bo is shared between many contexts, and so you are marking it as > available in every one. May I suggest looking for issues in > https://patchwork.freedesktop.org/series/27719/ and help bring that series to > fruition. Thanks Chris, we'll get back. > -Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence for Android OS v4.2
> -Original Message- > From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > Sent: Tuesday, July 25, 2017 11:22 PM > To: Marathe, Yogesh > Cc: Wu, Zhongmin ; ML mesa-dev d...@lists.freedesktop.org>; Kondapally, Kalyan > ; Antognolli, Rafael > ; Gao, Shuo ; Tomasz Figa > ; Chris Wilson ; Eric > Engestrom ; Rob Herring ; Daniel > Stone ; Varad Gautam > ; Liu, Zhiquan ; Frank > Binns ; Brendan King ; > Martin Peres > Subject: Re: [PATCH 2/2] i965: Queue the buffer with a sync fence for Android > OS v4.2 > > On 25 July 2017 at 15:30, Marathe, Yogesh > wrote: > > Emil, > > > >> -Original Message- > >> From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > >> Sent: Tuesday, July 25, 2017 7:46 PM > >> To: Wu, Zhongmin > >> Cc: ML mesa-dev ; Kondapally, Kalyan > >> ; Marathe, Yogesh > >> ; Antognolli, Rafael > >> ; Gao, Shuo ; Tomasz > >> Figa ; Chris Wilson ; > >> Eric Engestrom ; Rob Herring ; > >> Daniel Stone ; Varad Gautam > >> ; Liu, Zhiquan ; > >> Frank Binns ; Brendan King > >> ; Martin Peres > >> > >> Subject: Re: [PATCH 2/2] i965: Queue the buffer with a sync fence for > >> Android OS v4.2 > >> > >> Hi Zhongmin, > >> > >> Thanks you for the update. There's a couple of important comments - > >> dri2_make_current + droid_window_enqueue_buffer. > >> The rest is just nitpiks. > >> > >> Tomasz, hats down for the immense help and guidance. > >> > >> On the potential performance hit (due to the extra fence), I think we > >> could do some tests before adding extra infra. > >> No obvious benchmarks come to mind - any suggestions? > >> > > > > Sorry to jump in, flatland is the one native application on android > > that tests this explicitly. It gives time required to render one frame > > of particular resolution without other services running. It’s a native > > app that comes with aosp. And we found this issue just because of that. > > > > App info - > > https://android.googlesource.com/platform/frameworks/native/+/master/c > > mds/flatland/README.txt Bug - > > https://bugs.freedesktop.org/show_bug.cgi?id=101655 > > > > I already tested this patch set with android and I can see scores not being > > that > great. > > May be this is the one we can use to profile this or I can continue to > > profile based on guidance here. > > > I meant a completely different thing: > > Don't bother with premature optimisations - see if/how much overhead of the > patch itself adds (ideally on most platforms). > Aka - test before/after. Which in the case of flatland is not possible, if I > understood you correctly. Yes, it won't be functional without patch. > > About the performance numbers in question - I hope you've looked at my > comment in 1/2. Yes. I saw that. > > -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence for Android OS v4.2
Emil,
> -Original Message-
> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> Sent: Tuesday, July 25, 2017 7:46 PM
> To: Wu, Zhongmin
> Cc: ML mesa-dev ; Kondapally, Kalyan
> ; Marathe, Yogesh
> ; Antognolli, Rafael
> ; Gao, Shuo ; Tomasz Figa
> ; Chris Wilson ; Eric
> Engestrom ; Rob Herring ; Daniel
> Stone ; Varad Gautam
> ; Liu, Zhiquan ; Frank
> Binns ; Brendan King ;
> Martin Peres
> Subject: Re: [PATCH 2/2] i965: Queue the buffer with a sync fence for Android
> OS v4.2
>
> Hi Zhongmin,
>
> Thanks you for the update. There's a couple of important comments -
> dri2_make_current + droid_window_enqueue_buffer.
> The rest is just nitpiks.
>
> Tomasz, hats down for the immense help and guidance.
>
> On the potential performance hit (due to the extra fence), I think we could do
> some tests before adding extra infra.
> No obvious benchmarks come to mind - any suggestions?
>
Sorry to jump in, flatland is the one native application on android that tests
this
explicitly. It gives time required to render one frame of particular resolution
without
other services running. It’s a native app that comes with aosp. And we found
this
issue just because of that.
App info -
https://android.googlesource.com/platform/frameworks/native/+/master/cmds/flatland/README.txt
Bug - https://bugs.freedesktop.org/show_bug.cgi?id=101655
I already tested this patch set with android and I can see scores not being
that great.
May be this is the one we can use to profile this or I can continue to profile
based on
guidance here.
> On 25 July 2017 at 03:07, Zhongmin Wu wrote:
> > Before we queued the buffer with a invalid fence (-1), it will make
> > some benchmarks failed to test such as flatland.
> >
> > Now we get the out fence during the flushing buffer and then pass it
> > to SurfaceFlinger in eglSwapbuffer function.
> >
> > v2: a) Also implement the fence in cancelBuffer.
> > b) The last sync fence is stored in drawable object
> >rather than brw context.
> > c) format clear.
> >
> > v3: a) Save the last fence fd in DRI Context object.
> > b) Return the last fence if the batch buffer is empty and
> >nothing to be flushed when _intel_batchbuffer_flush_fence
> > c) Add the new interface in vbtl to set the retrieve fence
> >
> > v3.1 a) close fd in the new vbtl interface on none Android platform
> >
> > v4: a) The last fence is saved in brw context.
> > b) The retrieve fd is for all the platform but not just Android
> > c) Add a uniform dri2 interface to initialize the surface.
> >
> > v4.1: a) make some changes of variable name.
> > b) the patch is breaked into two patches.
> >
> > v4.2: a) Add a deinit interface for surface to clear the out fence
> >
> > Change-Id: Ided54d2e193cde73a6f0feb36ac1c0056e4958f2
> > Signed-off-by: Zhongmin Wu
> > ---
> > src/egl/drivers/dri2/egl_dri2.c | 51
> > +++
> > src/egl/drivers/dri2/egl_dri2.h |8 +
> > src/egl/drivers/dri2/platform_android.c | 12 ---
> > src/egl/drivers/dri2/platform_drm.c |3 +-
> > src/egl/drivers/dri2/platform_surfaceless.c |3 +-
> > src/egl/drivers/dri2/platform_wayland.c |3 +-
> > src/egl/drivers/dri2/platform_x11.c |3 +-
> > src/egl/drivers/dri2/platform_x11_dri3.c|3 +-
> > 8 files changed, 77 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/egl_dri2.c
> > b/src/egl/drivers/dri2/egl_dri2.c index 020a0bc..ffd3a8a 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.c
> > +++ b/src/egl/drivers/dri2/egl_dri2.c
> > @@ -1307,6 +1307,32 @@ dri2_destroy_context(_EGLDriver *drv,
> _EGLDisplay *disp, _EGLContext *ctx)
> > return EGL_TRUE;
> > }
> >
> > +EGLBoolean
> > +dri2_surf_init(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> nit: s/dri2_surf_init/dri2_init_surface/
>
> > +_EGLConfig *conf, const EGLint *attrib_list) {
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + dri2_surf->out_fence_fd = -1;
> > + return _eglInitSurface(surf, dpy, type, conf, attrib_list); }
> > +
> > +static void
> > +dri2_surface_set_out_fence( _EGLSurface *surf, int fence_fd)
> nit: s/dri2_surface_set_out_fence/dri2_surface_set_out_fence_fd/
>
> > +{
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + if (dri2_surf->out_fence_fd >=0)
> nit: space between = and 0
>
> > + close(dri2_surf->out_fence_fd);
> > +
&
Re: [Mesa-dev] [PATCH V3] i965 : Optimize atom state flag checks
Hi Matt, Sorry for late reply, please see below.
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Matt Turner
> Sent: Saturday, July 22, 2017 12:12 AM
> To: Muthukumar, Aravindan
> Cc: mesa-dev@lists.freedesktop.org; Marathe, Yogesh
>
> Subject: Re: [Mesa-dev] [PATCH V3] i965 : Optimize atom state flag checks
>
> On 07/21, aravindan.muthuku...@intel.com wrote:
> >From: Aravindan Muthukumar
> >
> >This patch improves CPI Rate(Cycles per Instruction) and branch miss
> >predict for i965. The function check_state() was showing CPI retired rate.
> >
> >Performance stats with android:
> >- CPI retired lowered by 28% (lower is better)
> >- Branch missprediction lowered by 13% (lower is better)
> >- 3DMark improved by 2%
> >
> >The dissassembly doesn't show difference, although above results were
> >observed with patch.
>
Yes this is true for V3 where we removed the function based on a review comment.
> If there's no difference in the assembly then whatever measurements you have
> taken must be noise.
>
No that's not guaranteed either. Lot of things still depend on how instructions
are
aligned, sometimes even changing linking order gives different results where
disassemblies of individual functions remain same.
> I applied the patch and inspected brw_state_upload.o. There are assembly
> differences. I can produce the same assembly as this patch by just pulling
> the if
> statement out of check_and_emit_atom() and into the caller. The replacement
> of check_state() with a macro is completely unnecessary.
>
>
> diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> b/src/mesa/drivers/dri/i965/brw_state_upload.c
> index acaa97ee7d..b163e1490d 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> @@ -439,14 +439,12 @@ merge_ctx_state(struct brw_context *brw, }
>
> static inline void
> -check_and_emit_atom(struct brw_context *brw,
> -struct brw_state_flags *state,
> -const struct brw_tracked_state *atom)
> +emit_atom(struct brw_context *brw,
> + struct brw_state_flags *state,
> + const struct brw_tracked_state *atom)
> {
> - if (check_state(state, &atom->dirty)) {
> - atom->emit(brw);
> - merge_ctx_state(brw, state);
> - }
> + atom->emit(brw);
> + merge_ctx_state(brw, state);
> }
>
> static inline void
> @@ -541,7 +539,9 @@ brw_upload_pipeline_state(struct brw_context *brw,
>const struct brw_tracked_state *atom = &atoms[i];
>struct brw_state_flags generated;
>
> - check_and_emit_atom(brw, &state, atom);
> + if (check_state(&state, &atom->dirty)) {
> +emit_atom(brw, &state, atom);
> + }
>
>accumulate_state(&examined, &atom->dirty);
>
> @@ -558,7 +558,9 @@ brw_upload_pipeline_state(struct brw_context *brw,
>for (i = 0; i < num_atoms; i++) {
>const struct brw_tracked_state *atom = &atoms[i];
>
> - check_and_emit_atom(brw, &state, atom);
> + if (check_state(&state, &atom->dirty)) {
> +emit_atom(brw, &state, atom);
> + }
>}
> }
>
>
> With that said, the assembly differences are not understandable to me.
> Why should extracting an if statement from a static inline function into the
> caller
> of that function cause any difference whatsoever?
Agreed, it shouldn't in case of static inline.
>
> I'm viewing the assembly differences with:
>
> wdiff -n \
> -w $'\033[30;41m' -x $'\033[0m' \
> -y $'\033[30;42m' -z $'\033[0m' \
> <(objdump -d brw_state_upload.o.before | sed -e 's/^.*\t//') \
> <(objdump -d brw_state_upload.o.wtf| sed -e 's/^.*\t//') | less -R
>
> and the only real difference is the movement of some call and jmp
> instructions.
>
> I cannot take the patch without some reasonable explanation for the change.
Ok I think this has been discussed already and we agree that there is no big
visible difference
in disassembly which can be pointed out for improvement. Although, as you said
this movement
of instructions can cause this. If with this patch instructions get cache
aligned that too can show
improvement. This is a busy function with bad CPI. Hence chosen for
optimization. Branch miss
predict is another metric. Do we want to consider all these or just
disassembly?
Let me make one more attempt, we clearly see icache misses for
brw_upload_pipelin
Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
> Just some style comments, feel free to ignore them.
>
Both comments are relevant, will address them in V3. Thanks Lionel.
> On 20/07/17 12:35, aravindan.muthuku...@intel.com wrote:
> > From: Aravindan Muthukumar
> >
> > This patch improves CPI Rate(Cycles per Instruction) and branch
> > mispredict for i965. The function check_state() was showing CPI
> > retired rate.
> >
> > Performance stats with android:
> > CPI retired lowered by 28% (lower is better) Branch missprediction
> > lowered by 13% (lower is better) 3DMark improved by 2%
> >
> > The dissassembly doesn't show difference, although above results were
> > observed with patch.
> >
> > Signed-off-by: Aravindan Muthukumar
> > Signedd-off-by: Yogesh Marathe
> > Tested-by: Asish
> > ---
> >
> > Changes since V1:
> > - Removed memset() change
> > - Changed commit message as per review comments
> >
> > src/mesa/drivers/dri/i965/brw_defines.h | 4
> > src/mesa/drivers/dri/i965/brw_state_upload.c | 12
> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> > b/src/mesa/drivers/dri/i965/brw_defines.h
> > index 2a8dbf8..8c9a510 100644
> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > @@ -1687,3 +1687,7 @@ enum brw_pixel_shader_coverage_mask_mode {
> > # define CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE (1 << 4)
> >
> > #endif
> > +
> > +/* Checking the state of mesa and brw before emitting atoms */
> > +#define CHECK_BRW_STATE(a,b) ((a.mesa & b.mesa) | (a.brw & b.brw))
> > +
> > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > index acaa97e..1c8b969 100644
> > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > @@ -443,10 +443,8 @@ check_and_emit_atom(struct brw_context *brw,
> > struct brw_state_flags *state,
> > const struct brw_tracked_state *atom)
> > {
> > - if (check_state(state, &atom->dirty)) {
> > atom->emit(brw);
> > merge_ctx_state(brw, state);
> > - }
>
> You might want to re-indent this.
> Also maybe that function can be rename since it won't check anything anymore.
>
> > }
> >
> > static inline void
> > @@ -541,7 +539,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
> > const struct brw_tracked_state *atom = &atoms[i];
> > struct brw_state_flags generated;
> >
> > - check_and_emit_atom(brw, &state, atom);
> > + /* Checking the state and emitting atoms */
> > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > +check_and_emit_atom(brw, &state, atom);
> > + }
> >
> > accumulate_state(&examined, &atom->dirty);
> >
> > @@ -558,7 +559,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
> > for (i = 0; i < num_atoms; i++) {
> > const struct brw_tracked_state *atom = &atoms[i];
> >
> > - check_and_emit_atom(brw, &state, atom);
> > + /* Checking the state and emitting atoms */
> > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > +check_and_emit_atom(brw, &state, atom);
> > + }
> > }
> > }
> >
>
>
> Why not replacing the last call to check_state() by CHECK_BRW_STATE() and get
> rid of that function altogether?
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
> -Original Message-
> From: Ian Romanick [mailto:i...@freedesktop.org]
> Sent: Friday, July 21, 2017 2:24 AM
> To: Marathe, Yogesh ; Muthukumar, Aravindan
> ; mesa-dev@lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
>
> On 07/20/2017 12:57 PM, Marathe, Yogesh wrote:
> > Ian,
> >
> >> -Original Message-
> >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> >> Behalf Of Ian Romanick
> >> Sent: Friday, July 21, 2017 12:33 AM
> >> To: Muthukumar, Aravindan ; mesa-
> >> d...@lists.freedesktop.org
> >> Subject: Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag
> >> checks
> >>
> >> On 07/20/2017 04:35 AM, aravindan.muthuku...@intel.com wrote:
> >>> From: Aravindan Muthukumar
> >>>
> >>> This patch improves CPI Rate(Cycles per Instruction) and branch
> >>> mispredict for i965. The function check_state() was showing CPI
> >>> retired rate.
> >>>
> >>> Performance stats with android:
> >>> CPI retired lowered by 28% (lower is better) Branch missprediction
> >>> lowered by 13% (lower is better) 3DMark improved by 2%
> >>>
> >>> The dissassembly doesn't show difference, although above results
> >>> were observed with patch.
> >>>
> >>> Signed-off-by: Aravindan Muthukumar
> >>> Signedd-off-by: Yogesh Marathe
> >>
> >> Signed-off-by
> >
> > Thanks. Will correct it. May I add you and all who commented as Reviewed-
> by?
> > I won't make a V3 for this since its a change in commit msg.
>
> No. You don't add someone's R-b unless they actually say "Reviewed-by".
> Various people still have issues with the content of this change.
>
Ok. Got that. Thanks.
> >>> Tested-by: Asish
> >>> ---
> >>>
> >>> Changes since V1:
> >>> - Removed memset() change
> >>> - Changed commit message as per review comments
> >>
> >> This information should be in the main part of the commit message.
> >>
> >
> > Sure.
> >
> >>>
> >>> src/mesa/drivers/dri/i965/brw_defines.h | 4
> >>> src/mesa/drivers/dri/i965/brw_state_upload.c | 12
> >>> 2 files changed, 12 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> >>> b/src/mesa/drivers/dri/i965/brw_defines.h
> >>> index 2a8dbf8..8c9a510 100644
> >>> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> >>> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> >>> @@ -1687,3 +1687,7 @@ enum brw_pixel_shader_coverage_mask_mode {
> #
> >>> define CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE (1 << 4)
> >>>
> >>> #endif
> >>> +
> >>> +/* Checking the state of mesa and brw before emitting atoms */
> >>> +#define CHECK_BRW_STATE(a,b) ((a.mesa & b.mesa) | (a.brw & b.brw))
> >>> +
> >>> diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> >>> b/src/mesa/drivers/dri/i965/brw_state_upload.c
> >>> index acaa97e..1c8b969 100644
> >>> --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> >>> +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> >>> @@ -443,10 +443,8 @@ check_and_emit_atom(struct brw_context *brw,
> >>> struct brw_state_flags *state,
> >>> const struct brw_tracked_state *atom) {
> >>> - if (check_state(state, &atom->dirty)) {
> >>>atom->emit(brw);
> >>>merge_ctx_state(brw, state);
> >>> - }
> >>> }
> >>>
> >>> static inline void
> >>> @@ -541,7 +539,10 @@ brw_upload_pipeline_state(struct brw_context
> *brw,
> >>>const struct brw_tracked_state *atom = &atoms[i];
> >>>struct brw_state_flags generated;
> >>>
> >>> - check_and_emit_atom(brw, &state, atom);
> >>> + /* Checking the state and emitting atoms */
> >>> + if (CHECK_BRW_STATE(state, atom->dirty)) {
> >>> +check_and_emit_atom(brw, &state, atom);
> >>> + }
> >>>
> >>>accumulate_state(&examined, &atom->dirty);
> >>>
> >>> @@ -558,7 +559,10 @@ brw_upload_pipeline_state(struct brw_context
> *brw,
> >>>for (i = 0; i < num_atoms; i++) {
> >>>const struct brw_tracked_state *atom = &atoms[i];
> >>>
> >>> - check_and_emit_atom(brw, &state, atom);
> >>> + /* Checking the state and emitting atoms */
> >>> + if (CHECK_BRW_STATE(state, atom->dirty)) {
> >>> +check_and_emit_atom(brw, &state, atom);
> >>> + }
> >>>}
> >>> }
> >>>
> >>>
> >>
> >> ___
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
Ian,
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Ian Romanick
> Sent: Friday, July 21, 2017 12:33 AM
> To: Muthukumar, Aravindan ; mesa-
> d...@lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
>
> On 07/20/2017 04:35 AM, aravindan.muthuku...@intel.com wrote:
> > From: Aravindan Muthukumar
> >
> > This patch improves CPI Rate(Cycles per Instruction) and branch
> > mispredict for i965. The function check_state() was showing CPI
> > retired rate.
> >
> > Performance stats with android:
> > CPI retired lowered by 28% (lower is better) Branch missprediction
> > lowered by 13% (lower is better) 3DMark improved by 2%
> >
> > The dissassembly doesn't show difference, although above results were
> > observed with patch.
> >
> > Signed-off-by: Aravindan Muthukumar
> > Signedd-off-by: Yogesh Marathe
>
> Signed-off-by
Thanks. Will correct it. May I add you and all who commented as Reviewed-by?
I won't make a V3 for this since its a change in commit msg.
>
> > Tested-by: Asish
> > ---
> >
> > Changes since V1:
> > - Removed memset() change
> > - Changed commit message as per review comments
>
> This information should be in the main part of the commit message.
>
Sure.
> >
> > src/mesa/drivers/dri/i965/brw_defines.h | 4
> > src/mesa/drivers/dri/i965/brw_state_upload.c | 12
> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> > b/src/mesa/drivers/dri/i965/brw_defines.h
> > index 2a8dbf8..8c9a510 100644
> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > @@ -1687,3 +1687,7 @@ enum brw_pixel_shader_coverage_mask_mode { #
> > define CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE (1 << 4)
> >
> > #endif
> > +
> > +/* Checking the state of mesa and brw before emitting atoms */
> > +#define CHECK_BRW_STATE(a,b) ((a.mesa & b.mesa) | (a.brw & b.brw))
> > +
> > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > index acaa97e..1c8b969 100644
> > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > @@ -443,10 +443,8 @@ check_and_emit_atom(struct brw_context *brw,
> > struct brw_state_flags *state,
> > const struct brw_tracked_state *atom) {
> > - if (check_state(state, &atom->dirty)) {
> >atom->emit(brw);
> >merge_ctx_state(brw, state);
> > - }
> > }
> >
> > static inline void
> > @@ -541,7 +539,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
> > const struct brw_tracked_state *atom = &atoms[i];
> > struct brw_state_flags generated;
> >
> > - check_and_emit_atom(brw, &state, atom);
> > + /* Checking the state and emitting atoms */
> > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > +check_and_emit_atom(brw, &state, atom);
> > + }
> >
> > accumulate_state(&examined, &atom->dirty);
> >
> > @@ -558,7 +559,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
> >for (i = 0; i < num_atoms; i++) {
> > const struct brw_tracked_state *atom = &atoms[i];
> >
> > - check_and_emit_atom(brw, &state, atom);
> > + /* Checking the state and emitting atoms */
> > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > +check_and_emit_atom(brw, &state, atom);
> > + }
> >}
> > }
> >
> >
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
Francisco,
> -Original Message-
> From: Francisco Jerez [mailto:curroje...@riseup.net]
> Sent: Friday, July 21, 2017 12:21 AM
> To: Marathe, Yogesh ; Muthukumar, Aravindan
> ; mesa-dev@lists.freedesktop.org
> Cc: Muthukumar, Aravindan
> Subject: RE: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
>
> "Marathe, Yogesh" writes:
>
> > Francisco,
> >
> >> -Original Message-
> >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> >> Behalf Of Francisco Jerez
> >> Sent: Thursday, July 20, 2017 10:51 PM
> >> To: Muthukumar, Aravindan ; mesa-
> >> d...@lists.freedesktop.org
> >> Cc: Muthukumar, Aravindan
> >> Subject: Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag
> >> checks
> >>
> >> aravindan.muthuku...@intel.com writes:
> >>
> >> > From: Aravindan Muthukumar
> >> >
> >> > This patch improves CPI Rate(Cycles per Instruction) and branch
> >> > mispredict for i965. The function check_state() was showing CPI
> >> > retired rate.
> >> >
> >> > Performance stats with android:
> >> > CPI retired lowered by 28% (lower is better) Branch missprediction
> >> > lowered by 13% (lower is better) 3DMark improved by 2%
> >> >
> >> > The dissassembly doesn't show difference, although above results
> >> > were observed with patch.
> >> >
> >>
> >> How did you determine that your results are not just statistical noise?
> >
> > No its not statistical noise. As commit msg mentions, we used metrics
> > CPI retired rate, utilization, branch miss predict as metrics, these can be
> measured using SEP on IA.
> > It essentially enables event based sampling and we can measure these through
> counters.
> >
>
> How much variance do these metrics have? (particularly the overall score of
> the
> benchmark) How many times did you run the benchmark?
>
2% to be exact, other stats are also present in commit message, the benchmark
was run at least 5 times before concluding and more than that during
experimenting.
> > When we did the analysis of tests we were running, we found
> > brw_upload_pipeline_state->check_state functions having bad CPI rates
> > and hence we made changed there. The intention was always to reduce
> > driver overhead, although this is miniscule effort.
> >
> >> Did you do some sort of significance testing? Which test,
> >> significance level and sample size did you use?
> >
> > Sorry this is not something we have done, we tested on android
> > functionality and perf only. Performance benchmark 3dmark and overall
> > stability of the android system were used as tests. Kindly let us know
> > if you have specific tests to be run and we would be happy to run that.
> >
> > What CPU did you get the numbers on?
> >
> > Broxton.
> >
> >>
> >> Thank you.
> >>
> >> > Signed-off-by: Aravindan Muthukumar
> >> >
> >> > Signedd-off-by: Yogesh Marathe
> >> > Tested-by: Asish
> >> > ---
> >> >
> >> > Changes since V1:
> >> > - Removed memset() change
> >> > - Changed commit message as per review comments
> >> >
> >> > src/mesa/drivers/dri/i965/brw_defines.h | 4
> >> > src/mesa/drivers/dri/i965/brw_state_upload.c | 12
> >> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> >> > b/src/mesa/drivers/dri/i965/brw_defines.h
> >> > index 2a8dbf8..8c9a510 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> >> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> >> > @@ -1687,3 +1687,7 @@ enum brw_pixel_shader_coverage_mask_mode
> { #
> >> > define CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE (1 << 4)
> >> >
> >> > #endif
> >> > +
> >> > +/* Checking the state of mesa and brw before emitting atoms */
> >> > +#define CHECK_BRW_STATE(a,b) ((a.mesa & b.mesa) | (a.brw & b.brw))
> >> > +
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> >> > b/src/mesa/drivers/dri/i965/brw_state_upload.c
> >> > index acaa97e..1c8b969 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> >> > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> &g
Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
Francisco,
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Francisco Jerez
> Sent: Thursday, July 20, 2017 10:51 PM
> To: Muthukumar, Aravindan ; mesa-
> d...@lists.freedesktop.org
> Cc: Muthukumar, Aravindan
> Subject: Re: [Mesa-dev] [PATCH V2] i965 : Optimize atom state flag checks
>
> aravindan.muthuku...@intel.com writes:
>
> > From: Aravindan Muthukumar
> >
> > This patch improves CPI Rate(Cycles per Instruction) and branch
> > mispredict for i965. The function check_state() was showing CPI
> > retired rate.
> >
> > Performance stats with android:
> > CPI retired lowered by 28% (lower is better) Branch missprediction
> > lowered by 13% (lower is better) 3DMark improved by 2%
> >
> > The dissassembly doesn't show difference, although above results were
> > observed with patch.
> >
>
> How did you determine that your results are not just statistical noise?
No its not statistical noise. As commit msg mentions, we used metrics CPI
retired rate,
utilization, branch miss predict as metrics, these can be measured using SEP on
IA.
It essentially enables event based sampling and we can measure these through
counters.
When we did the analysis of tests we were running, we found
brw_upload_pipeline_state->check_state functions having bad CPI rates and hence
we made changed there. The intention was always to reduce driver overhead,
although
this is miniscule effort.
> Did you do some sort of significance testing? Which test, significance level
> and
> sample size did you use?
Sorry this is not something we have done, we tested on android functionality and
perf only. Performance benchmark 3dmark and overall stability of the android
system
were used as tests. Kindly let us know if you have specific tests to be run and
we would
be happy to run that.
What CPU did you get the numbers on?
Broxton.
>
> Thank you.
>
> > Signed-off-by: Aravindan Muthukumar
> > Signedd-off-by: Yogesh Marathe
> > Tested-by: Asish
> > ---
> >
> > Changes since V1:
> > - Removed memset() change
> > - Changed commit message as per review comments
> >
> > src/mesa/drivers/dri/i965/brw_defines.h | 4
> > src/mesa/drivers/dri/i965/brw_state_upload.c | 12
> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> > b/src/mesa/drivers/dri/i965/brw_defines.h
> > index 2a8dbf8..8c9a510 100644
> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > @@ -1687,3 +1687,7 @@ enum brw_pixel_shader_coverage_mask_mode { #
> > define CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE (1 << 4)
> >
> > #endif
> > +
> > +/* Checking the state of mesa and brw before emitting atoms */
> > +#define CHECK_BRW_STATE(a,b) ((a.mesa & b.mesa) | (a.brw & b.brw))
> > +
> > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > index acaa97e..1c8b969 100644
> > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > @@ -443,10 +443,8 @@ check_and_emit_atom(struct brw_context *brw,
> > struct brw_state_flags *state,
> > const struct brw_tracked_state *atom) {
> > - if (check_state(state, &atom->dirty)) {
> >atom->emit(brw);
> >merge_ctx_state(brw, state);
> > - }
> > }
> >
> > static inline void
> > @@ -541,7 +539,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
> > const struct brw_tracked_state *atom = &atoms[i];
> > struct brw_state_flags generated;
> >
> > - check_and_emit_atom(brw, &state, atom);
> > + /* Checking the state and emitting atoms */
> > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > +check_and_emit_atom(brw, &state, atom);
> > + }
> >
> > accumulate_state(&examined, &atom->dirty);
> >
> > @@ -558,7 +559,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
> >for (i = 0; i < num_atoms; i++) {
> > const struct brw_tracked_state *atom = &atoms[i];
> >
> > - check_and_emit_atom(brw, &state, atom);
> > + /* Checking the state and emitting atoms */
> > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > +check_and_emit_atom(brw, &state, atom);
> > + }
> >}
> > }
> >
> > --
> > 2.7.4
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965 : Performance Improvement
> -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > Of Eero Tamminen > Sent: Friday, July 14, 2017 2:20 PM > To: mesa-dev@lists.freedesktop.org > Subject: Re: [Mesa-dev] [PATCH] i965 : Performance Improvement > > Hi, > > On 14.07.2017 09:38, Marathe, Yogesh wrote: > [...] > >>>> The only reason I could see this helping is if check_state() wasn't > >>>> inlined, but a release build with -O2 definitely inlines both > >>>> check_and_emit_atom() and check_state(). > >>>> > >>>> Are you using GCC? What are your CFLAGS? -O2? I hope you're not > >>>> trying to optimize a debug build... > >>> > >>> Yes we are using O2 and its clang on android and it's not debug. > >> > >> Okay. I just built with Clang 4.0.1 and -O2 and both check_state and > >> check_and_emit_atom() are inlined into the atom loop in > >> brw_upload_pipeline_state(). > >> > >> So I'm still not sure how this would improve anything. > > > > Yes, the improvement is not huge per say but we essentially see CPI > > and cpu utilization is coming down with this. We also see slightly > > improved scores on graphics benchmarks, particularly 3dmark with the > > patch. If this was optimized out by compiler we shouldn't have seen > > the difference on same build with and without patch. We'll confirm the clang > version. > > > > I think this removes branch instructions least and being in busy path > > this will have an impact, provided compiler doesn't do it, as you rightly > mentioned. > > Did you disassemble the produced code to verify that it improved things like > you > thought it to improve? No, This didn’t appear through code disassembly so we didn’t do it after too, we continued with same CPI measurement, basically cpu counters, but now that this is brought up we should do it. > > The reason why ask, is that just doing changes to unrelated parts of code can > sometimes improve performance because it changes code size and therefore > impacts how things end up being mapped to memory and cached. > Good point. Thanks, we'll check that angle, too. Changes aren't unrelated though. > (In some cases I've see several percent performance increases and drops even > from LD_PRELOADing a random, unused library to a process.) > > > - Eero > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965 : Performance Improvement
> -Original Message-
> From: Kenneth Graunke [mailto:kenn...@whitecape.org]
> Sent: Friday, July 14, 2017 11:20 AM
> To: Marathe, Yogesh
> Cc: mesa-dev@lists.freedesktop.org; Muthukumar, Aravindan
>
> Subject: Re: [Mesa-dev] [PATCH] i965 : Performance Improvement
>
> On Thursday, July 13, 2017 9:49:40 PM PDT Marathe, Yogesh wrote:
> > Kenneth,
> >
> > > -Original Message-
> > > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> > > Behalf Of Kenneth Graunke
> > > Sent: Friday, July 14, 2017 10:05 AM
> > > To: mesa-dev@lists.freedesktop.org
> > > Cc: Muthukumar, Aravindan
> > > Subject: Re: [Mesa-dev] [PATCH] i965 : Performance Improvement
> > >
> > > On Thursday, July 13, 2017 9:09:09 PM PDT
> > > aravindan.muthuku...@intel.com
> > > wrote:
> > > > From: Aravindan M
> > >
> > > The commit title should be something like, "i965: Optimize atom
> > > state flag checks" rather than a generic "Performance Improvement"
> > >
> > > > This patch improves CPI Rate(Cycles per Instruction) and CPU time
> > > > utilization for i965. The functions check_state and
> > > > brw_pipeline_state_finished was found poor CPU utilization from
> > > > performance analysis.
> > >
> > > Need actual data here, or show assembly quality improvements.
> > >
> > > > Change-Id: I17c7e719a16e222764217a0e67b4482748537b67
> > > > Signed-off-by: Aravindan M
> > > > Reviewed-by: Yogesh M
> > > > Tested-by: Asish
> > > > ---
> > > > src/mesa/drivers/dri/i965/brw_defines.h | 3 +++
> > > > src/mesa/drivers/dri/i965/brw_state_upload.c | 14 +++---
> > > > 2 files changed, 14 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> > > > b/src/mesa/drivers/dri/i965/brw_defines.h
> > > > index a4794c6..60f88ca 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > > > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > > > @@ -1681,3 +1681,6 @@ enum brw_pixel_shader_coverage_mask_mode
> {
> > > > # define GEN8_L3CNTLREG_ALL_ALLOC_MASK INTEL_MASK(31, 25)
> > > >
> > > > #endif
> > > > +
> > > > +/* Checking the state of mesa and brw before emitting atoms */
> > > > +#define CHECK_BRW_STATE(a,b) ((a.mesa & b.mesa) | (a.brw &
> > > > +b.brw))
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > > > b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > > > index 5e82c1b..434decf 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > > > @@ -515,7 +515,10 @@ brw_upload_pipeline_state(struct brw_context
> *brw,
> > > > const struct brw_tracked_state *atom = &atoms[i];
> > > > struct brw_state_flags generated;
> > > >
> > > > - check_and_emit_atom(brw, &state, atom);
> > > > + /* Checking the state and emitting the atoms */
> > > > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > > > +check_and_emit_atom(brw, &state, atom);
> > > > + }
> > > >
> > > > accumulate_state(&examined, &atom->dirty);
> > > >
> > > > @@ -532,7 +535,10 @@ brw_upload_pipeline_state(struct brw_context
> *brw,
> > > >for (i = 0; i < num_atoms; i++) {
> > > > const struct brw_tracked_state *atom = &atoms[i];
> > > >
> > > > - check_and_emit_atom(brw, &state, atom);
> > > > + /* Checking the state and emitting the atoms */
> > > > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > > > +check_and_emit_atom(brw, &state, atom);
> > > > + }
> > >
> > > This doesn't make any sense...the very first thing
> > > check_and_emit_atom() does is call check_state(), which does the
> > > exact thing your CHECK_BRW_STATE macro does. So you're just needlessly
> checking the same thing twice.
> > >
> >
> > Sorry Kenneth, This is incomplete patch. The original patch that I
> > reviewed also had if check removed as below
> >
> > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> &g
Re: [Mesa-dev] [PATCH] i965 : Performance Improvement
Kenneth,
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Kenneth Graunke
> Sent: Friday, July 14, 2017 10:05 AM
> To: mesa-dev@lists.freedesktop.org
> Cc: Muthukumar, Aravindan
> Subject: Re: [Mesa-dev] [PATCH] i965 : Performance Improvement
>
> On Thursday, July 13, 2017 9:09:09 PM PDT aravindan.muthuku...@intel.com
> wrote:
> > From: Aravindan M
>
> The commit title should be something like, "i965: Optimize atom state flag
> checks" rather than a generic "Performance Improvement"
>
> > This patch improves CPI Rate(Cycles per Instruction) and CPU time
> > utilization for i965. The functions check_state and
> > brw_pipeline_state_finished was found poor CPU utilization from
> > performance analysis.
>
> Need actual data here, or show assembly quality improvements.
>
> > Change-Id: I17c7e719a16e222764217a0e67b4482748537b67
> > Signed-off-by: Aravindan M
> > Reviewed-by: Yogesh M
> > Tested-by: Asish
> > ---
> > src/mesa/drivers/dri/i965/brw_defines.h | 3 +++
> > src/mesa/drivers/dri/i965/brw_state_upload.c | 14 +++---
> > 2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> > b/src/mesa/drivers/dri/i965/brw_defines.h
> > index a4794c6..60f88ca 100644
> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > @@ -1681,3 +1681,6 @@ enum brw_pixel_shader_coverage_mask_mode {
> > # define GEN8_L3CNTLREG_ALL_ALLOC_MASK INTEL_MASK(31, 25)
> >
> > #endif
> > +
> > +/* Checking the state of mesa and brw before emitting atoms */
> > +#define CHECK_BRW_STATE(a,b) ((a.mesa & b.mesa) | (a.brw & b.brw))
> > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > index 5e82c1b..434decf 100644
> > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c
> > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
> > @@ -515,7 +515,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
> > const struct brw_tracked_state *atom = &atoms[i];
> > struct brw_state_flags generated;
> >
> > - check_and_emit_atom(brw, &state, atom);
> > + /* Checking the state and emitting the atoms */
> > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > +check_and_emit_atom(brw, &state, atom);
> > + }
> >
> > accumulate_state(&examined, &atom->dirty);
> >
> > @@ -532,7 +535,10 @@ brw_upload_pipeline_state(struct brw_context *brw,
> >for (i = 0; i < num_atoms; i++) {
> > const struct brw_tracked_state *atom = &atoms[i];
> >
> > - check_and_emit_atom(brw, &state, atom);
> > + /* Checking the state and emitting the atoms */
> > + if (CHECK_BRW_STATE(state, atom->dirty)) {
> > +check_and_emit_atom(brw, &state, atom);
> > + }
>
> This doesn't make any sense...the very first thing check_and_emit_atom() does
> is call check_state(), which does the exact thing your CHECK_BRW_STATE macro
> does. So you're just needlessly checking the same thing twice.
>
Sorry Kenneth, This is incomplete patch. The original patch that I reviewed
also had
if check removed as below
--- a/src/mesa/drivers/dri/i965/brw_state_upload.c
+++ b/src/mesa/drivers/dri/i965/brw_state_upload.c
@@ -417,10 +417,8 @@ check_and_emit_atom(struct brw_context *brw,
struct brw_state_flags *state,
const struct brw_tracked_state *atom)
{
- if (check_state(state, &atom->dirty)) {
atom->emit(brw);
merge_ctx_state(brw, state);
- }
}
Aravindan will push another set.
> The only reason I could see this helping is if check_state() wasn't inlined,
> but a
> release build with -O2 definitely inlines both check_and_emit_atom() and
> check_state().
>
> Are you using GCC? What are your CFLAGS? -O2? I hope you're not trying to
> optimize a debug build...
>
Yes we are using O2 and its clang on android and it's not debug.
> >}
> > }
> >
> > @@ -567,7 +573,9 @@ brw_pipeline_state_finished(struct brw_context *brw,
> > brw->state.pipelines[i].mesa |= brw->NewGLState;
> > brw->state.pipelines[i].brw |= brw->ctx.NewDriverState;
> >} else {
> > - memset(&brw->state.pipelines[i], 0, sizeof(struct
> > brw_state_flags));
> > + /* Avoiding the memset with initialization */
> > + brw->state.pipelines[i].mesa = 0;
> > + brw->state.pipelines[i].brw = 0ull;
> >}
> > }
>
> This is a separate change.
>
> I'm also not seeing why it's useful:
>
> Assembly before (GCC 7.1.1, x86_64, -O2 -fno-omit-frame-pointer):
>
> 003e0380 :
> 3e0380: 66 0f ef c0 pxor %xmm0,%xmm0
> 3e0384: 55 push %rbp
> 3e0385: 8b 87 20 52 02 00 mov0x25220(%rdi),%eax
> 3e038b: c7 87 20 52 02 00 00movl $0x0,0x25220(%rdi)
> 3e0392: 00 00 00
> 3
Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS
> -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > Of Tomasz Figa > Sent: Tuesday, July 11, 2017 9:59 PM > To: Marathe, Yogesh > Cc: Gao, Shuo ; Liu, Zhiquan ; > Kondapally, Kalyan ; Chad Versace > ; Eric Engestrom ; Emil > Velikov ; Wu, Zhongmin > ; Kenneth Graunke ; Rob > Clark ; Widawsky, Benjamin > ; ML mesa-dev d...@lists.freedesktop.org>; Kristian H . Kristensen > ; Timothy Arceri > Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: > Queue the buffer with a sync fence for Android OS > > Hi Yogesh, > > On Wed, Jul 12, 2017 at 1:09 AM, Marathe, Yogesh > wrote: > > Hello Figa, Few caveats on that approach > > (I'm Tomasz, by the way) My bad Tomasz. > >> Queue the buffer with a sync fence for Android OS > >> > >> Now for real, sorry guys... (Seriously gmail why you do this to me.) > >> > >> -chuanbo.w...@intel.com (bounces) > >> +"Gao, Shuo" (forgot to add originally, sorry) > >> > >> On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa > wrote: > >> > Hi Zhongmin, > >> > > >> > On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin > >> > > >> wrote: > >> >> By the way, > >> >> > >> >> For cancelBuffer, sorry I forget such function, thanks for notice. > >> >> It should > >> also pass the same fence fd as the queuebuffer. > >> >> > >> >> And Yogesh, you mentioned the gallium, is it another platform > >> >> supported > by > >> mesa ? I am sorry I have no idea about this, could you please help > >> to check this ? > >> >> > >> >> I think we can co-work with mesa team to work out an acceptable > >> >> fix which > >> can meet the requirement of Android without any break on other platforms. > >> > > >> > One thing needs clarifying here. Release fences from EGL are _not_ > >> > a requirement. It is an optional feature. Android compliance suites > >> > pass fully without Android sync fence support in Mesa at all. > >> > > >> > Other than that, it's been taking long enough and I agree that we > >> > should finally wire both acquire and release fence support in EGL > >> > and related drivers. Otherwise we can forget about getting good > >> > user experience on Android. > >> > > >> > On a technical side, the EGL change needs to take into account that > >> > not all drivers support fences and so it needs to have a fallback > >> > to old behavior for those which don't. > >> > > >> > Other than that, correct me if I'm wrong, but could we just use the > >> > DRI2 fence extension instead of adding some custom callbacks? I can > >> > see that a normal client request to create a sync fence would end > >> > up calling dri2_dpy->fence->create_fence_fd() (if it's present) [1]. > >> > Could we do the same? > > > > May be here we need to look at complete sequence eglCreateSyncKHR -> > > _eglCreateSync -> dri2_create_sync, as eglCreateSyncKHR seems entry > > point and its doing attrib/type checks before reaching > > dri2_create_sync(). Also, dri2_create_sync is static, can't be called > > directly, there needs to be an entry point / interface. > > > > I think you misunderstood my suggestion. I didn't mean dri2_create_sync(), but > rather using the DRI2 fence extension directly, just as dri2_create_sync() > does. > You can access dri2_egl_display from Android EGL code and in fact it already > uses other extensions like this. Sorry, I'm still searching around. To try this out, can you please specify, which functions did you mean by DRI2 fence extension? An example within EGL code would help. Please note we need to call these from platform_android.c finally. > > Best regards, > Tomasz > > >> > > >> > [1] > >> > https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/eg > >> > l_d > >> > ri2.c#n2772 > >> > > >> > + Kristian, Chad and Dominik who have been looking into sync fence > >> > integration with our EGL drivers. > >> > > >> > Best regards, > >> > Tomasz > >> > > >> >> > >> >> -Original Message- > >> >> From: Wu, Zhongmin > >> >> Sent: Tuesday, July 11, 2017 8:40 > >
Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS
Hello Figa, Few caveats on that approach > -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > Of Tomasz Figa > Sent: Tuesday, July 11, 2017 8:58 PM > To: Wu, Zhongmin > Cc: Gao, Shuo ; Liu, Zhiquan ; > ML mesa-dev ; Emil Velikov > ; Chad Versace ; Eric > Engestrom ; Marathe, Yogesh > ; Rob Clark ; Kenneth > Graunke ; Widawsky, Benjamin > ; Kondapally, Kalyan > ; Kristian H . Kristensen > ; Timothy Arceri > Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: > Queue the buffer with a sync fence for Android OS > > Now for real, sorry guys... (Seriously gmail why you do this to me.) > > -chuanbo.w...@intel.com (bounces) > +"Gao, Shuo" (forgot to add originally, sorry) > > On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa wrote: > > Hi Zhongmin, > > > > On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin > wrote: > >> By the way, > >> > >> For cancelBuffer, sorry I forget such function, thanks for notice. It > >> should > also pass the same fence fd as the queuebuffer. > >> > >> And Yogesh, you mentioned the gallium, is it another platform supported > >> by > mesa ? I am sorry I have no idea about this, could you please help to check > this > ? > >> > >> I think we can co-work with mesa team to work out an acceptable fix which > can meet the requirement of Android without any break on other platforms. > > > > One thing needs clarifying here. Release fences from EGL are _not_ a > > requirement. It is an optional feature. Android compliance suites pass > > fully without Android sync fence support in Mesa at all. > > > > Other than that, it's been taking long enough and I agree that we > > should finally wire both acquire and release fence support in EGL and > > related drivers. Otherwise we can forget about getting good user > > experience on Android. > > > > On a technical side, the EGL change needs to take into account that > > not all drivers support fences and so it needs to have a fallback to > > old behavior for those which don't. > > > > Other than that, correct me if I'm wrong, but could we just use the > > DRI2 fence extension instead of adding some custom callbacks? I can > > see that a normal client request to create a sync fence would end up > > calling dri2_dpy->fence->create_fence_fd() (if it's present) [1]. > > Could we do the same? May be here we need to look at complete sequence eglCreateSyncKHR -> _eglCreateSync -> dri2_create_sync, as eglCreateSyncKHR seems entry point and its doing attrib/type checks before reaching dri2_create_sync(). Also, dri2_create_sync is static, can't be called directly, there needs to be an entry point / interface. > > > > [1] > > https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/egl_d > > ri2.c#n2772 > > > > + Kristian, Chad and Dominik who have been looking into sync fence > > integration with our EGL drivers. > > > > Best regards, > > Tomasz > > > >> > >> -Original Message- > >> From: Wu, Zhongmin > >> Sent: Tuesday, July 11, 2017 8:40 > >> To: 'Emil Velikov' ; Marathe, Yogesh > >> > >> Cc: Widawsky, Benjamin ; Liu, Zhiquan > >> ; Eric Engestrom ; Rob > >> Clark ; Tomasz Figa ; > >> Kenneth Graunke ; Kondapally, Kalyan > >> ; ML mesa-dev > >> ; Timothy Arceri > >> ; Chuanbo Weng > >> Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] > >> i965: Queue the buffer with a sync fence for Android OS > >> > >> Hi Emil and Yogesh > >> Thank you for your comments, and thanks Yogesh for giving the > >> detailed explanations > >> > >> > >> And according to the document of Android below > (https://source.android.com/devices/graphics/arch-bq-gralloc): > >> > >> Recent Android devices support the sync framework, which enables the > system to do nifty things when combined with hardware components that can > manipulate graphics data asynchronously. For example, a producer can submit a > series of OpenGL ES drawing commands and then enqueue the output buffer > before rendering completes. The buffer is accompanied by a fence that signals > when the contents are ready. > >> > >> > >> I think the things is very clear, that is if the rendering is completed > >> already > when we call queueBuffer() in mesa ? If not, we should queue the buffer > with a > fence wh
Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS
Hello Emil, My two cents since I too spent some time on this.
> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Emil Velikov
> Sent: Monday, July 10, 2017 4:41 PM
> To: Wu, Zhongmin
> Cc: Widawsky, Benjamin ; Liu, Zhiquan
> ; Eric Engestrom ; Rob Clark
> ; Tomasz Figa ; Kenneth
> Graunke ; Kondapally, Kalyan
> ; ML mesa-dev d...@lists.freedesktop.org>; Timothy Arceri ; Chuanbo
> Weng
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
> Queue the buffer with a sync fence for Android OS
>
> Hi Zhongmin Wu,
>
> Above all, a bit of a disclaimer: I'm by no means an expert on the topic so
> take
> the following with a pinch of salt.
>
> On 10 July 2017 at 03:11, Zhongmin Wu wrote:
> > Before we queued the buffer with a invalid fence (-1), it will make
> > some benchmarks failed to test such as flatland.
> >
> > Now we get the out fence during the flushing buffer and then pass it
> > to SurfaceFlinger in eglSwapbuffer function.
> >
> Having a closer look it seems that the issue can be summarised as follows:
> - flatland intercepts/interacts ANativeWindow::{de,}queueBuffer (how about
> ANativeWindow::cancelBuffer?)
> - the program expects that a sync fd is available for both dequeue and queue
>
> At the same time:
> - the ANativeWindow documentation does _not_ state such requirement
> - even if it did, that will be somewhat wrong, since
> ANativeWindow::queueBuffer is called by eglSwapBuffers() Where the latter
> documentation clearly states - "... performs an implicit flush ... glFlush ...
> vgFlush"
>
> My take is that if flatland/Android framework does want an explicit sync
> point it
> should insert one with the EGL API.
> There could be alternative solutions, but the proposed patch seems wrong
> IMHO.
In fact, I could work this around in producer (Surface::queueBuffer) by
ignoring the (-1)
passed and by creating a sync using egl APIs. I see two problems with that.
- Before getting a fd using eglDupNativeFenceFDANDROID(), you need a glFlush(),
this costs additional cycles for each queueBuffer transaction on each
BufferItem and
I believe fd is also signaled due to this. (so I don’t know what we'll get
by waiting on
that fd on consumer side).
- AFAIK, the whole idea of explicit sync revolves around being able to pass fds
created
by driver between processes and this one breaks that chain. If we work this
around in
upper layers, explicit sync feature will have to be fixed for every other lib
that may use
lib mesa underneath.
For these reasons, I still believe we should fix it here. Of course, you and
Rob have very
valid points on cancelBuffer and about not breaking gallium respectively, those
need to
be taken care of.
>
> Regards,
> Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Explicit sync tests on android
Can someone please confirm if we can claim explicit sync support on android with mesa today? If yes, which tests can be used to verify this other than flatland? Sorry for typo in last email below (corrected). Regards, Yogesh. From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of Marathe, Yogesh Sent: Friday, June 23, 2017 3:50 PM To: mesa-dev@lists.freedesktop.org Subject: [Mesa-dev] Explicit sync tests on android Hi Rob, Is there any test other than _flatland_ on android with which we can confirm explicit sync support? Regards, Yogesh. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Explicit sync tests on android
Hi Rob, Is there any test other than _flatland_ on android with which we can confirm explicit sync support can be verified? Regards, Yogesh. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] egl/android: fence_fd being forced to -1
> -Original Message- > From: Chad Versace [mailto:chadvers...@chromium.org] > On Tue 20 Jun 2017, Marathe, Yogesh wrote: > > > > From the framework's perspective, at least from the comments in > > > aosp://system/core/include/system/window.h [1], it's legal to call > > > ANativeWindow::queueBuffer with fenceFd=-1 if implicit sync is enabled. > > > And, as far as I know, implicity sync is still enabled on Intel > > > unless you set a flag in execbuffer. > > > > > > Yogesh and Tapani, are you disabling explicit sync for any > > > execbuffer ioctl's on android-ia? If so, please point me to some > > > code. If not, then this definitely feels like a NOTOURBUG. > > > > I think you meant implicit sync in the question, no we are not disabling it. > > Right. I meant "are you disabling implicit sync...". > > > > > As I look at the code further, it appears that the native application > > depends upon GLConsumer client's fence. This fence is expected to be > > coming from buffer producer (ANativeWindow/Surface in this case) > > through a BufferQueue, buffers in this buffer queue have fence fd > > forced to -1, before they were enqueued. If producer finds fence as -1 > > during > enqueue it just sets mFenceFd = Fence::NO_FENCE. > > Isn't this a problem when buffer is acquired by client which depends upon > fence? > > In the API's that I have encountered while working with Android's sync > framework, fenceFd=-1 always implies that the buffer is ready for access. The > spec for Google's Vulkan Android winsys extension also document fenceFd=-1 > this way. And EGL_ANDROID_native_fence_sync documents it this way also. > > Perhaps, (but I doubt it), as you proceed farther up the stack of the Android > framework, farther away from drivers and the HALs and closer to the Java, the > documented behavior changes for fences. But I have little knowledge of those > upper framework layers. I think actually that's what is happening, fence's purpose changed at that native application. It's not about buffer availability. It's trying to use fence fd from GL client to calculate time elapsed while its doing frames; assuming fence fd is always valid and has meaning to it in terms of sw_sync timeline and sync_pts. Fence::getSignalTime() from Fence.cpp will never give a valid outcome if egl driver forces fd = -1 https://android.googlesource.com/platform/frameworks/native/+/master/libs/ui/Fence.cpp ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] egl/android: fence_fd being forced to -1
> -Original Message- > On Thu 15 Jun 2017, Eric Engestrom wrote: > > On Thursday, 2017-06-15 16:17:20 -0400, Rob Clark wrote: > > > On Thu, Jun 15, 2017 at 1:17 PM, Tapani Pälli > wrote: > > > > On 06/15/2017 07:57 PM, Rob Clark wrote: > > > > > > > > On Thu, Jun 15, 2017 at 12:04 PM, Tapani Pälli > > > > > > > > wrote: > > > > > > > > On 06/15/2017 06:52 PM, Rob Clark wrote: > > > > > > > > On Thu, Jun 15, 2017 at 9:59 AM, Eric Engestrom > > > > wrote: > > > > > > > > On Thursday, 2017-06-15 13:27:06 +, Marathe, Yogesh wrote: > > > > > > > > Hello, > > > > > > > > I'm tyring to run flatland native app on android. It apparantly > > > > fails because of a fence issue. > > > > while debuging further it is observed that > > > > droid_window_enqueue_buffer() is forcing fence_fd =-1. > > > > > > > > Yogesh, can you describe a bit more what "fails" means? What > > > > sequence of gl calls, for example, is it making? > > > > > > > > > > > > The problem described shortly: Flatland uses timestamps from Fence > > > > objects for calculating time (using getSignalTime API) and in case > > > > of having -1 from producer (Mesa) we will end up having same > > > > timestamp for startFence and endFence (since both are > > > > Fence::NO_FENCE) and thus flatland will keep running forever as it > > > > thinks no time has been passed between 2 fences. It is stuck in a > > > > loop where it tries to find how many frames are required so that driver > > > > will > spend certain amount of time doing it. > > > > > > > > hmm, any idea what the getSignalTime() API sits on top of? I > > > > don't think we have such a capability with fence fd's.. > > > > > > > > > > > > I don't know much of libsync but it uses libsync functions > > > > 'sync_fence_info' > > > > and 'sync_pt_info' to retrieve data from fence fd. > > > > > > > > > > oh, yeah, upstream does look like it supports the SYNC_IOC_FILE_INFO > > > ioctl. So I guess someone did actually think this through ;-) > > > > If memory serves, Gustavo Padovan did most of that work :) > > Hi, I wrote the comment :) The comment is ancient, and predates the existence > of sync_file in the kernel and Mesa. That comment seems to be coming from window.h :), thanks for sync_file input. > > From the framework's perspective, at least from the comments in > aosp://system/core/include/system/window.h [1], it's legal to call > ANativeWindow::queueBuffer with fenceFd=-1 if implicit sync is enabled. > And, as far as I know, implicity sync is still enabled on Intel unless you > set a flag > in execbuffer. > > Yogesh and Tapani, are you disabling explicit sync for any execbuffer ioctl's > on > android-ia? If so, please point me to some code. If not, then this > definitely feels > like a NOTOURBUG. I think you meant implicit sync in the question, no we are not disabling it. As I look at the code further, it appears that the native application depends upon GLConsumer client's fence. This fence is expected to be coming from buffer producer (ANativeWindow/Surface in this case) through a BufferQueue, buffers in this buffer queue have fence fd forced to -1, before they were enqueued. If producer finds fence as -1 during enqueue it just sets mFenceFd = Fence::NO_FENCE. Isn't this a problem when buffer is acquired by client which depends upon fence? > > [1]: https://android.googlesource.com/platform/system/core/+/android- > 7.1.1_r43/include/system/window.h#572 > > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] egl/android: fence_fd being forced to -1
Hello Chad, Any comments from you on this topic? Regards, Yogesh. > -Original Message- > > > > On Thursday, 2017-06-15 13:27:06 +, Marathe, Yogesh wrote: > > > Hello, > > > > > > I'm tyring to run flatland native app on android. It apparantly > > > fails because of a > > fence issue. > > > while debuging further it is observed that > > > droid_window_enqueue_buffer() is forcing fence_fd =-1. > > > > I assume you've read the comment on the line above the one you mentioned? > > Yes I read this, although what's not clear to me is the case that not covered > in > comments i.e. when a valid fd needs to be used. I will wait for Chad's > comments. > Thanks Eric. > > > > >/* Queue the buffer without a sync fence. This informs the ANativeWindow > > * that it may access the buffer immediately. > > * > > * From ANativeWindow::dequeueBuffer: > > * > > *The fenceFd argument specifies a libsync fence file descriptor for > > *a fence that must signal before the buffer can be accessed. If > > *the buffer can be accessed immediately then a value of -1 should > > *be used. The caller must not use the file descriptor after it > > *is passed to queueBuffer, and the ANativeWindow implementation > > *is responsible for closing it. > > */ > >int fence_fd = -1; > >dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer, > > fence_fd); > > > > > Whats the expectation here if app wants to use fence sync? > > > > > > If we want to have this native app working with the lib where > > > exactly fence should be created / populated, it should be with buffer > producer, right? > > > > I don't know this code personally, so I Cc'ed Chad, who wrote this > > code in commit bfe28b8d93 (albeit 5 years ago, so he might not > > remember all of it) and Tapani who reviewed it. > > Yeah this app also seems to be around since then. > > > > > > > > > BTW, We can't/ don't want to change the flatland app! > > > > > > Regards, > > > Yogesh. > > > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] egl/android: fence_fd being forced to -1
> -Original Message- > From: Eric Engestrom [mailto:eric.engest...@imgtec.com] > Sent: Thursday, June 15, 2017 7:30 PM > To: Marathe, Yogesh > Cc: mesa-dev@lists.freedesktop.org; Chad Versace > ; Palli, Tapani > Subject: Re: [Mesa-dev] egl/android: fence_fd being forced to -1 > > On Thursday, 2017-06-15 13:27:06 +, Marathe, Yogesh wrote: > > Hello, > > > > I'm tyring to run flatland native app on android. It apparantly fails > > because of a > fence issue. > > while debuging further it is observed that > > droid_window_enqueue_buffer() is forcing fence_fd =-1. > > I assume you've read the comment on the line above the one you mentioned? Yes I read this, although what's not clear to me is the case that not covered in comments i.e. when a valid fd needs to be used. I will wait for Chad's comments. Thanks Eric. > >/* Queue the buffer without a sync fence. This informs the ANativeWindow > * that it may access the buffer immediately. > * > * From ANativeWindow::dequeueBuffer: > * > *The fenceFd argument specifies a libsync fence file descriptor for > *a fence that must signal before the buffer can be accessed. If > *the buffer can be accessed immediately then a value of -1 should > *be used. The caller must not use the file descriptor after it > *is passed to queueBuffer, and the ANativeWindow implementation > *is responsible for closing it. > */ >int fence_fd = -1; >dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer, > fence_fd); > > > Whats the expectation here if app wants to use fence sync? > > > > If we want to have this native app working with the lib where exactly > > fence should be created / populated, it should be with buffer producer, > > right? > > I don't know this code personally, so I Cc'ed Chad, who wrote this code in > commit bfe28b8d93 (albeit 5 years ago, so he might not remember all of it) and > Tapani who reviewed it. Yeah this app also seems to be around since then. > > > > > BTW, We can't/ don't want to change the flatland app! > > > > Regards, > > Yogesh. > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] egl/android: fence_fd being forced to -1
> -Original Message- > From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > Sent: Thursday, June 15, 2017 7:24 PM > To: Marathe, Yogesh > Cc: mesa-dev@lists.freedesktop.org > Subject: Re: [Mesa-dev] egl/android: fence_fd being forced to -1 > > On 15 June 2017 at 14:27, Marathe, Yogesh > wrote: > > Hello, > > > > I'm tyring to run flatland native app on android. It apparantly fails > > because of a > fence issue. > > while debuging further it is observed that > > droid_window_enqueue_buffer() is forcing fence_fd =-1. Whats the > expectation here if app wants to use fence sync? > > > > If we want to have this native app working with the lib where exactly > > fence should be created / populated, it should be with buffer producer, > > right? > > > > BTW, We can't/ don't want to change the flatland app! > > > If the app is doing something illegal, I doubt people will be extra happy in > pushing workarounds in Mesa. > > But above all - please check that your Mesa version has commit > 6f21b5601cc1260eac53f65c8941b3aa66d0f5e9. Yes I do have this patch already. > Additionally you may want to try this extra patch [1] although I'm not 100% > sure > how much these will help. > > IIRC we had a handful of other Android patches you might also want to > skim/test [2] Thanks for these pointers Emil, I will look at these patches. > > -Emil > > [1] https://patchwork.freedesktop.org/patch/154705/ > [2] > https://patchwork.freedesktop.org/project/mesa/patches/?submitter=&state= > &q=android&archive=&delegate= > It's kind of a long list, but I'll cleanup most of them shortly. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] egl/android: fence_fd being forced to -1
Hello, I'm tyring to run flatland native app on android. It apparantly fails because of a fence issue. while debuging further it is observed that droid_window_enqueue_buffer() is forcing fence_fd =-1. Whats the expectation here if app wants to use fence sync? If we want to have this native app working with the lib where exactly fence should be created / populated, it should be with buffer producer, right? BTW, We can't/ don't want to change the flatland app! Regards, Yogesh. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] i965: disable_throttling
Hi, > -Original Message- > Hi, > > On 09.06.2017 09:21, Marathe, Yogesh wrote: > > I’m looking forward to set this brw->disable_throttling to true, I’m > > actually observing adverse effect on performance benchmarks after I > > force set that to true. What’s the expectation from disable_throttling > > here? > > By default Mesa throttles its buffer swaps so that compositor is able to > easier > insert its own batches to better place in GPU batch queue. > > If throttling is disabled, compositor needs to discard more frames as > obsolete, > because benchmark is running too many frames forward of it. > > Normally having no throttling means that: > * composited performance looks worse to the user, because only some of the > rendered frames end up on screen > * benchmark runs faster because compositor composes less frames, i.e. > takes less of the system bandwidth > Thanks Eero. Essentially, you are saying having disable_trottling=true is actually _not_ beneficial from compositor's perspective where GPU is not matching speeds with that of benchmark. I will leave it to default then. > > > BTW, this is on android. > > On TDP constrained machines, power management can do funky things. When > in doubt, use fixed GPU & CPU speeds are re-test. > Alright. > > > - Eero > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] i965: disable_throttling
Hi, I'm looking forward to set this brw->disable_throttling to true, I'm actually observing adverse effect on performance benchmarks after I force set that to true. What's the expectation from disable_throttling here? BTW, this is on android. Regards, Yogesh. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
