Re: [Mesa-dev] [PATCH RFC 2/2] dri: Pass a __DRIcontext to ->set_damage_region()
Hi Boris, On Tue, 1 Oct 2019 at 09:25, Boris Brezillon wrote: > On Mon, 2 Sep 2019 16:32:01 +0200 Michel Dänzer wrote: > > On 2019-08-30 7:00 p.m., Boris Brezillon wrote: > > > So, next question is, do you think it's acceptable to pass a > > > DRIcontext here, and if not, do you have any idea how to solve this > > > problem? > > > > Hmm, not sure. Maybe it would be better to explicitly pass in the > > __DRIimage* to which the damage region applies? > > Sorry, for the late reply. I had a look at this proposal and I don't see > how passing a __DRIimage object would help. There's this comment [1] > that makes me think passing a drawable is the right thing to do, but at > the same time I'm not sure how to rework the logic to make it work > without having access to the pipe_context (sounds like an invasive > change to me). > So, I suggest that we revert [2] and [3] until we find a proper > solution to address the problem. > > Daniel, Qiang, are you okay with that? Yeah, I'm fine with that. I can't remember the details of why passing a DRIimage directly doesn't help. Writing that down somewhere would be good, I think. Cheers, Daniel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH RFC 2/2] dri: Pass a __DRIcontext to ->set_damage_region()
On Mon, 2 Sep 2019 16:32:01 +0200
Michel Dänzer wrote:
> On 2019-08-30 7:00 p.m., Boris Brezillon wrote:
> >
> > So, next question is, do you think it's acceptable to pass a
> > DRIcontext here, and if not, do you have any idea how to solve this
> > problem?
>
> Hmm, not sure. Maybe it would be better to explicitly pass in the
> __DRIimage* to which the damage region applies?
>
>
Sorry, for the late reply. I had a look at this proposal and I don't see
how passing a __DRIimage object would help. There's this comment [1]
that makes me think passing a drawable is the right thing to do, but at
the same time I'm not sure how to rework the logic to make it work
without having access to the pipe_context (sounds like an invasive
change to me).
So, I suggest that we revert [2] and [3] until we find a proper
solution to address the problem.
Daniel, Qiang, are you okay with that?
[1]https://elixir.bootlin.com/mesa/latest/source/src/mesa/state_tracker/st_manager.c#L197
[2]492ffbed63a2 ("st/dri2: Implement DRI2bufferDamageExtension")
[3]65ae86b85422 ("panfrost: Add support for KHR_partial_update()")
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH RFC 2/2] dri: Pass a __DRIcontext to ->set_damage_region()
On 2019-08-30 7:00 p.m., Boris Brezillon wrote: > > So, next question is, do you think it's acceptable to pass a > DRIcontext here, and if not, do you have any idea how to solve this > problem? Hmm, not sure. Maybe it would be better to explicitly pass in the __DRIimage* to which the damage region applies? -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH RFC 2/2] dri: Pass a __DRIcontext to ->set_damage_region()
On Fri, 30 Aug 2019 18:56:52 +0200
Michel Dänzer wrote:
> On 2019-08-30 6:52 p.m., Boris Brezillon wrote:
> > On Fri, 30 Aug 2019 18:43:59 +0200
> > Michel Dänzer wrote:
> >
> >>> diff --git a/include/GL/internal/dri_interface.h
> >>> b/include/GL/internal/dri_interface.h
> >>> index 4c60d349ddd5..e04bed689219 100644
> >>> --- a/include/GL/internal/dri_interface.h
> >>> +++ b/include/GL/internal/dri_interface.h
> >>> @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
> >>> *
> >>> * Used to implement EGL_KHR_partial_update.
> >>> *
> >>> +* \param ctx context
> >>> * \param drawable affected drawable
> >>> * \param nrects number of rectangles provided
> >>> * \param rectsthe array of rectangles, lower-left origin
> >>> */
> >>> - void (*set_damage_region)(__DRIdrawable *drawable, unsigned int
> >>> nrects,
> >>> - int *rects);
> >>> + void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
> >>> + unsigned int nrects, int *rects);
> >>> };
> >>
> >> This would break the DRI2_BufferDamage extension version 1 ABI. You'd
> >> need to either add a new hook like set_damage_region2 and bump
> >> __DRI2_BUFFER_DAMAGE_VERSION (and make sure that's handled correctly
> >> everywhere), or add a new extension instead.
> >
> > I thought this change was only impacting the internal API, but maybe
> > I'm missing something.
>
> include/GL/internal/dri_interface.h defines the DRI driver ABI, which
> must be kept backwards compatible.
Okay.
>
>
> > In any case, this extension has been merged recently (mesa-19.2.0-rc1),
> > so maybe we can fix it before 19.2 is released to avoid creating
> > ->set_damage_region2().
>
> Ah, yes. I misinterpreted gitk's output, thinking it had already been
> introduced in 19.1. Sorry for the false alarm.
Great! So, next question is, do you think it's acceptable to pass a
DRIcontext here, and if not, do you have any idea how to solve this
problem?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH RFC 2/2] dri: Pass a __DRIcontext to ->set_damage_region()
On 2019-08-30 6:52 p.m., Boris Brezillon wrote:
> On Fri, 30 Aug 2019 18:43:59 +0200
> Michel Dänzer wrote:
>
>>> diff --git a/include/GL/internal/dri_interface.h
>>> b/include/GL/internal/dri_interface.h
>>> index 4c60d349ddd5..e04bed689219 100644
>>> --- a/include/GL/internal/dri_interface.h
>>> +++ b/include/GL/internal/dri_interface.h
>>> @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
>>> *
>>> * Used to implement EGL_KHR_partial_update.
>>> *
>>> +* \param ctx context
>>> * \param drawable affected drawable
>>> * \param nrects number of rectangles provided
>>> * \param rectsthe array of rectangles, lower-left origin
>>> */
>>> - void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
>>> - int *rects);
>>> + void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
>>> + unsigned int nrects, int *rects);
>>> };
>>
>> This would break the DRI2_BufferDamage extension version 1 ABI. You'd
>> need to either add a new hook like set_damage_region2 and bump
>> __DRI2_BUFFER_DAMAGE_VERSION (and make sure that's handled correctly
>> everywhere), or add a new extension instead.
>
> I thought this change was only impacting the internal API, but maybe
> I'm missing something.
include/GL/internal/dri_interface.h defines the DRI driver ABI, which
must be kept backwards compatible.
> In any case, this extension has been merged recently (mesa-19.2.0-rc1),
> so maybe we can fix it before 19.2 is released to avoid creating
> ->set_damage_region2().
Ah, yes. I misinterpreted gitk's output, thinking it had already been
introduced in 19.1. Sorry for the false alarm.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH RFC 2/2] dri: Pass a __DRIcontext to ->set_damage_region()
On Fri, 30 Aug 2019 18:43:59 +0200
Michel Dänzer wrote:
> On 2019-08-29 1:54 p.m., Boris Brezillon wrote:
> > So we can call st_validate_state() from dri2_set_damage_region() in
> > order to update the BACK_LEFT attachement before using it. If we don't
> > do that, the resource passed to pipe_screen->set_damage_region() might
> > be wrong.
> >
> > Signed-off-by: Boris Brezillon
> > ---
> > Hi,
> >
> > I honestly don't know if this is the right thing to do. Passing a
> > context for somethings that we decided to bind to a surface (and
> > the underlying resources attached to it) seems wrong, but at the same
> > time I don't see any other option to sync the gallium drawable state
> > with the EGL DRI surface one.
> >
> > Any ideas/suggestions?
> >
> > Regards,
> >
> > Boris
> > ---
> > include/GL/internal/dri_interface.h | 5 +++--
> > src/egl/drivers/dri2/egl_dri2.c | 7 +--
> > src/gallium/state_trackers/dri/dri2.c | 11 ++-
> > 3 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/GL/internal/dri_interface.h
> > b/include/GL/internal/dri_interface.h
> > index 4c60d349ddd5..e04bed689219 100644
> > --- a/include/GL/internal/dri_interface.h
> > +++ b/include/GL/internal/dri_interface.h
> > @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
> > *
> > * Used to implement EGL_KHR_partial_update.
> > *
> > +* \param ctx context
> > * \param drawable affected drawable
> > * \param nrects number of rectangles provided
> > * \param rectsthe array of rectangles, lower-left origin
> > */
> > - void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
> > - int *rects);
> > + void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
> > + unsigned int nrects, int *rects);
> > };
>
> This would break the DRI2_BufferDamage extension version 1 ABI. You'd
> need to either add a new hook like set_damage_region2 and bump
> __DRI2_BUFFER_DAMAGE_VERSION (and make sure that's handled correctly
> everywhere), or add a new extension instead.
>
>
I thought this change was only impacting the internal API, but maybe
I'm missing something.
In any case, this extension has been merged recently (mesa-19.2.0-rc1),
so maybe we can fix it before 19.2 is released to avoid creating
->set_damage_region2().
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH RFC 2/2] dri: Pass a __DRIcontext to ->set_damage_region()
On 2019-08-29 1:54 p.m., Boris Brezillon wrote:
> So we can call st_validate_state() from dri2_set_damage_region() in
> order to update the BACK_LEFT attachement before using it. If we don't
> do that, the resource passed to pipe_screen->set_damage_region() might
> be wrong.
>
> Signed-off-by: Boris Brezillon
> ---
> Hi,
>
> I honestly don't know if this is the right thing to do. Passing a
> context for somethings that we decided to bind to a surface (and
> the underlying resources attached to it) seems wrong, but at the same
> time I don't see any other option to sync the gallium drawable state
> with the EGL DRI surface one.
>
> Any ideas/suggestions?
>
> Regards,
>
> Boris
> ---
> include/GL/internal/dri_interface.h | 5 +++--
> src/egl/drivers/dri2/egl_dri2.c | 7 +--
> src/gallium/state_trackers/dri/dri2.c | 11 ++-
> 3 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/include/GL/internal/dri_interface.h
> b/include/GL/internal/dri_interface.h
> index 4c60d349ddd5..e04bed689219 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
> *
> * Used to implement EGL_KHR_partial_update.
> *
> +* \param ctx context
> * \param drawable affected drawable
> * \param nrects number of rectangles provided
> * \param rectsthe array of rectangles, lower-left origin
> */
> - void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
> - int *rects);
> + void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
> + unsigned int nrects, int *rects);
> };
This would break the DRI2_BufferDamage extension version 1 ABI. You'd
need to either add a new hook like set_damage_region2 and bump
__DRI2_BUFFER_DAMAGE_VERSION (and make sure that's handled correctly
everywhere), or add a new extension instead.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH RFC 2/2] dri: Pass a __DRIcontext to ->set_damage_region()
On Thu, 29 Aug 2019 13:54:16 +0200 Boris Brezillon
said:
These 2 do improve things, but once you start doing BindFramebuffer()'s as part
of the render cycle ... its back to rendering artifacts. I am not quite sure
exactly what yet. I need to capture some output and traces to get a better idea
of what is going on.
> So we can call st_validate_state() from dri2_set_damage_region() in
> order to update the BACK_LEFT attachement before using it. If we don't
> do that, the resource passed to pipe_screen->set_damage_region() might
> be wrong.
>
> Signed-off-by: Boris Brezillon
> ---
> Hi,
>
> I honestly don't know if this is the right thing to do. Passing a
> context for somethings that we decided to bind to a surface (and
> the underlying resources attached to it) seems wrong, but at the same
> time I don't see any other option to sync the gallium drawable state
> with the EGL DRI surface one.
>
> Any ideas/suggestions?
>
> Regards,
>
> Boris
> ---
> include/GL/internal/dri_interface.h | 5 +++--
> src/egl/drivers/dri2/egl_dri2.c | 7 +--
> src/gallium/state_trackers/dri/dri2.c | 11 ++-
> 3 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/include/GL/internal/dri_interface.h
> b/include/GL/internal/dri_interface.h index 4c60d349ddd5..e04bed689219 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
> *
> * Used to implement EGL_KHR_partial_update.
> *
> +* \param ctx context
> * \param drawable affected drawable
> * \param nrects number of rectangles provided
> * \param rectsthe array of rectangles, lower-left origin
> */
> - void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
> - int *rects);
> + void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
> + unsigned int nrects, int *rects);
> };
>
> /*@}*/
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index ea8ae5d44c56..797a76dab13c 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1812,11 +1812,14 @@ dri2_set_damage_region(_EGLDriver *drv, _EGLDisplay
> *disp, _EGLSurface *surf, {
> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> __DRIdrawable *drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
> + _EGLContext *ctx = _eglGetCurrentContext();
> + __DRIcontext *dri_ctx = ctx ? dri2_egl_context(ctx)->dri_context : NULL;
>
> - if (!dri2_dpy->buffer_damage || !
> dri2_dpy->buffer_damage->set_damage_region)
> + if (!dri2_dpy->buffer_damage || !
> dri2_dpy->buffer_damage->set_damage_region ||
> + !dri_ctx)
>return EGL_FALSE;
>
> - dri2_dpy->buffer_damage->set_damage_region(drawable, n_rects, rects);
> + dri2_dpy->buffer_damage->set_damage_region(dri_ctx, drawable, n_rects,
> rects); return EGL_TRUE;
> }
>
> diff --git a/src/gallium/state_trackers/dri/dri2.c
> b/src/gallium/state_trackers/dri/dri2.c index 11ce19787249..bab503046510
> 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -36,6 +36,7 @@
> #include "util/u_format.h"
> #include "util/u_debug.h"
> #include "state_tracker/drm_driver.h"
> +#include "state_tracker/st_atom.h"
> #include "state_tracker/st_cb_bufferobjects.h"
> #include "state_tracker/st_cb_fbo.h"
> #include "state_tracker/st_cb_texture.h"
> @@ -1869,9 +1870,17 @@ static const __DRI2interopExtension
> dri2InteropExtension = {
> * \brief the DRI2bufferDamageExtension set_damage_region method
> */
> static void
> -dri2_set_damage_region(__DRIdrawable *dPriv, unsigned int nrects, int *rects)
> +dri2_set_damage_region(__DRIcontext *_ctx, __DRIdrawable *dPriv,
> + unsigned int nrects, int *rects)
> {
> struct dri_drawable *drawable = dri_drawable(dPriv);
> + struct st_context *st = (struct st_context *)dri_context(_ctx)->st;
> +
> + /* Make sure drawable->textures[ST_ATTACHMENT_BACK_LEFT] is up-to-date
> +* before using it.
> +*/
> + st_validate_state(st, ST_PIPELINE_UPDATE_FRAMEBUFFER);
> +
> struct pipe_resource *resource =
> drawable->textures[ST_ATTACHMENT_BACK_LEFT]; struct pipe_screen *screen =
> resource->screen; struct pipe_box *boxes = NULL;
> --
> 2.21.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
- Codito, ergo sum - "I code, therefore I am" --
Carsten Haitzler - ras...@rasterman.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
