* Re: [PATCH] drm: rcar-du: track dma-buf fences
2018-04-03 18:53 ` Laurent Pinchart
@ 2018-04-03 20:25 ` Daniel Vetter
2018-04-04 10:59 ` Ucan, Emre (ADITG/ESB)
2018-04-05 6:39 ` Ucan, Emre (ADITG/ESB)
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2018-04-03 20:25 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: dri-devel, Emre Ucan
On Tue, Apr 3, 2018 at 8:53 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hello Emre,
>
> Thank you for the patch.
>
> On Tuesday, 3 April 2018 12:14:33 EEST Emre Ucan wrote:
>> We have to check dma-buf reservation objects
>> of our framebuffers before we use them.
>> Otherwise, another driver might be writing
>> on the same buffer which we are using.
>> This would cause visible tearing effects
>> on display.
>>
>> We can use existing atomic helper functions
>> to solve this problem.
>>
>> Signed-off-by: Emre Ucan <eucan@de.adit-jv.com>
>> ---
>> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 ++
>> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 20 ++++++++++++++++++++
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 0329b35..f3da3d1 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> @@ -255,6 +255,8 @@ static void rcar_du_atomic_commit_tail(struct
>> drm_atomic_state *old_state) {
>> struct drm_device *dev = old_state->dev;
>>
>> + drm_atomic_helper_wait_for_fences(dev, old_state, false);
>> +
>
> The commit_tail() function in drm_atomic_helper.c, which calls our
> atomic_commit_tail() implementation, already calls
> drm_atomic_helper_wait_for_fences(). Why is there a need to duplicate the call
> here ?
>
>> /* Apply the atomic update. */
>> drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> drm_atomic_helper_commit_planes(dev, old_state,
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c3..482e23c 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> @@ -18,12 +18,16 @@
>> #include <drm/drm_fb_cma_helper.h>
>> #include <drm/drm_gem_cma_helper.h>
>> #include <drm/drm_plane_helper.h>
>> +#include <drm/drm_atomic.h>
>>
>> #include <linux/bitops.h>
>> #include <linux/dma-mapping.h>
>> +#include <linux/dma-fence.h>
>> +#include <linux/dma-buf.h>
>> #include <linux/of_platform.h>
>> #include <linux/scatterlist.h>
>> #include <linux/videodev2.h>
>> +#include <linux/reservation.h>
>>
>> #include <media/vsp1.h>
>>
>> @@ -203,6 +207,20 @@ static void rcar_du_vsp_plane_setup(struct
>> rcar_du_vsp_plane *plane) plane->index, &cfg);
>> }
>>
>> +static void rcar_du_vsp_set_fence_for_plane(struct drm_plane_state *state)
>> +{
>> + struct drm_gem_cma_object *gem;
>> + struct dma_buf *dma_buf;
>> + struct dma_fence *fence;
>> +
>> + gem = drm_fb_cma_get_gem_obj(state->fb, 0);
>> + dma_buf = gem->base.dma_buf;
>> + if (dma_buf) {
>> + fence = reservation_object_get_excl_rcu(dma_buf->resv);
>> + drm_atomic_set_fence_for_plane(state, fence);
>
> Unless I'm mistaken this is used for implicit fencing only. What is your use
> case, wouldn't it be better for userspace to use explicit fencing as that is
> the fence model that has been selected for display ?
Implicit fencing is very much still a thing on most X and wayland
setups. There's a push to eventually make everything explicit, but
it'll take a while I think.
-Daniel
>
>> + }
>> +}
>
> This looks very similar to drm_gem_fb_prepare_fb(), couldn't you use that
> function instead ?
>
>> static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
>> struct drm_plane_state *state)
>> {
>> @@ -237,6 +255,8 @@ static int rcar_du_vsp_plane_prepare_fb(struct drm_plane
>> *plane, }
>> }
>>
>> + rcar_du_vsp_set_fence_for_plane(state);
>> +
>> return 0;
>>
>> fail:
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] drm: rcar-du: track dma-buf fences
2018-04-03 18:53 ` Laurent Pinchart
2018-04-03 20:25 ` Daniel Vetter
@ 2018-04-04 10:59 ` Ucan, Emre (ADITG/ESB)
2018-04-05 10:41 ` Daniel Vetter
2018-04-05 6:39 ` Ucan, Emre (ADITG/ESB)
2 siblings, 1 reply; 6+ messages in thread
From: Ucan, Emre (ADITG/ESB) @ 2018-04-04 10:59 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: dri-devel
Hello Laurent
Thank you for your review.
> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Dienstag, 3. April 2018 20:53
> To: Ucan, Emre (ADITG/ESB)
> Cc: dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH] drm: rcar-du: track dma-buf fences
>
> Hello Emre,
>
> Thank you for the patch.
>
> On Tuesday, 3 April 2018 12:14:33 EEST Emre Ucan wrote:
> > We have to check dma-buf reservation objects
> > of our framebuffers before we use them.
> > Otherwise, another driver might be writing
> > on the same buffer which we are using.
> > This would cause visible tearing effects
> > on display.
> >
> > We can use existing atomic helper functions
> > to solve this problem.
> >
> > Signed-off-by: Emre Ucan <eucan@de.adit-jv.com>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 ++
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 20 ++++++++++++++++++++
> > 2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 0329b35..f3da3d1 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -255,6 +255,8 @@ static void rcar_du_atomic_commit_tail(struct
> > drm_atomic_state *old_state) {
> > struct drm_device *dev = old_state->dev;
> >
> > + drm_atomic_helper_wait_for_fences(dev, old_state, false);
> > +
>
> The commit_tail() function in drm_atomic_helper.c, which calls our
> atomic_commit_tail() implementation, already calls
> drm_atomic_helper_wait_for_fences(). Why is there a need to duplicate the
> call
> here ?
You are right. I will remove it in second version.
>
> > /* Apply the atomic update. */
> > drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > drm_atomic_helper_commit_planes(dev, old_state,
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c3..482e23c 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > @@ -18,12 +18,16 @@
> > #include <drm/drm_fb_cma_helper.h>
> > #include <drm/drm_gem_cma_helper.h>
> > #include <drm/drm_plane_helper.h>
> > +#include <drm/drm_atomic.h>
> >
> > #include <linux/bitops.h>
> > #include <linux/dma-mapping.h>
> > +#include <linux/dma-fence.h>
> > +#include <linux/dma-buf.h>
> > #include <linux/of_platform.h>
> > #include <linux/scatterlist.h>
> > #include <linux/videodev2.h>
> > +#include <linux/reservation.h>
> >
> > #include <media/vsp1.h>
> >
> > @@ -203,6 +207,20 @@ static void rcar_du_vsp_plane_setup(struct
> > rcar_du_vsp_plane *plane) plane->index, &cfg);
> > }
> >
> > +static void rcar_du_vsp_set_fence_for_plane(struct drm_plane_state
> *state)
> > +{
> > + struct drm_gem_cma_object *gem;
> > + struct dma_buf *dma_buf;
> > + struct dma_fence *fence;
> > +
> > + gem = drm_fb_cma_get_gem_obj(state->fb, 0);
> > + dma_buf = gem->base.dma_buf;
> > + if (dma_buf) {
> > + fence = reservation_object_get_excl_rcu(dma_buf->resv);
> > + drm_atomic_set_fence_for_plane(state, fence);
>
> Unless I'm mistaken this is used for implicit fencing only. What is your use
> case, wouldn't it be better for userspace to use explicit fencing as that is
> the fence model that has been selected for display ?
We are using Weston on Renesas R-Car H3 SoC. I am using GPU rendered client buffers
directly as scanout buffer for the display. Weston is not using explicit fencing.
>
> > + }
> > +}
>
> This looks very similar to drm_gem_fb_prepare_fb(), couldn't you use that
> function instead ?
Description of drm_gem_fb_prepare_fb() function states that it can be
used as the &drm_plane_helper_funcs.prepare_fb hook. But we have
our own hook function which is called rcar_du_vsp_plane_prepare_fb().
Therefore, I was not sure if it is correct to use drm_gem_fb_prepare_fb()
inside our hook function.
I will use it in the second version nevertheless.
>
> > static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> > struct drm_plane_state *state)
> > {
> > @@ -237,6 +255,8 @@ static int rcar_du_vsp_plane_prepare_fb(struct
> drm_plane
> > *plane, }
> > }
> >
> > + rcar_du_vsp_set_fence_for_plane(state);
> > +
> > return 0;
> >
> > fail:
>
> --
> Regards,
>
> Laurent Pinchart
>
>
Best Regards,
Emre Ucan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm: rcar-du: track dma-buf fences
2018-04-04 10:59 ` Ucan, Emre (ADITG/ESB)
@ 2018-04-05 10:41 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2018-04-05 10:41 UTC (permalink / raw)
To: Ucan, Emre (ADITG/ESB); +Cc: Laurent Pinchart, dri-devel
On Wed, Apr 4, 2018 at 12:59 PM, Ucan, Emre (ADITG/ESB)
<eucan@de.adit-jv.com> wrote:
> Hello Laurent
>
> Thank you for your review.
>
>> -----Original Message-----
>> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
>> Sent: Dienstag, 3. April 2018 20:53
>> To: Ucan, Emre (ADITG/ESB)
>> Cc: dri-devel@lists.freedesktop.org
>> Subject: Re: [PATCH] drm: rcar-du: track dma-buf fences
>>
>> Hello Emre,
>>
>> Thank you for the patch.
>>
>> On Tuesday, 3 April 2018 12:14:33 EEST Emre Ucan wrote:
>> > We have to check dma-buf reservation objects
>> > of our framebuffers before we use them.
>> > Otherwise, another driver might be writing
>> > on the same buffer which we are using.
>> > This would cause visible tearing effects
>> > on display.
>> >
>> > We can use existing atomic helper functions
>> > to solve this problem.
>> >
>> > Signed-off-by: Emre Ucan <eucan@de.adit-jv.com>
>> > ---
>> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 ++
>> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 20 ++++++++++++++++++++
>> > 2 files changed, 22 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 0329b35..f3da3d1 100644
>> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> > @@ -255,6 +255,8 @@ static void rcar_du_atomic_commit_tail(struct
>> > drm_atomic_state *old_state) {
>> > struct drm_device *dev = old_state->dev;
>> >
>> > + drm_atomic_helper_wait_for_fences(dev, old_state, false);
>> > +
>>
>> The commit_tail() function in drm_atomic_helper.c, which calls our
>> atomic_commit_tail() implementation, already calls
>> drm_atomic_helper_wait_for_fences(). Why is there a need to duplicate the
>> call
>> here ?
>
> You are right. I will remove it in second version.
You can use it in your own hook. Patch to update the kerneldoc to
clarify that would be great.
-Daniel
>
>>
>> > /* Apply the atomic update. */
>> > drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> > drm_atomic_helper_commit_planes(dev, old_state,
>> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c3..482e23c 100644
>> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> > @@ -18,12 +18,16 @@
>> > #include <drm/drm_fb_cma_helper.h>
>> > #include <drm/drm_gem_cma_helper.h>
>> > #include <drm/drm_plane_helper.h>
>> > +#include <drm/drm_atomic.h>
>> >
>> > #include <linux/bitops.h>
>> > #include <linux/dma-mapping.h>
>> > +#include <linux/dma-fence.h>
>> > +#include <linux/dma-buf.h>
>> > #include <linux/of_platform.h>
>> > #include <linux/scatterlist.h>
>> > #include <linux/videodev2.h>
>> > +#include <linux/reservation.h>
>> >
>> > #include <media/vsp1.h>
>> >
>> > @@ -203,6 +207,20 @@ static void rcar_du_vsp_plane_setup(struct
>> > rcar_du_vsp_plane *plane) plane->index, &cfg);
>> > }
>> >
>> > +static void rcar_du_vsp_set_fence_for_plane(struct drm_plane_state
>> *state)
>> > +{
>> > + struct drm_gem_cma_object *gem;
>> > + struct dma_buf *dma_buf;
>> > + struct dma_fence *fence;
>> > +
>> > + gem = drm_fb_cma_get_gem_obj(state->fb, 0);
>> > + dma_buf = gem->base.dma_buf;
>> > + if (dma_buf) {
>> > + fence = reservation_object_get_excl_rcu(dma_buf->resv);
>> > + drm_atomic_set_fence_for_plane(state, fence);
>>
>> Unless I'm mistaken this is used for implicit fencing only. What is your use
>> case, wouldn't it be better for userspace to use explicit fencing as that is
>> the fence model that has been selected for display ?
>
> We are using Weston on Renesas R-Car H3 SoC. I am using GPU rendered client buffers
> directly as scanout buffer for the display. Weston is not using explicit fencing.
>
>>
>> > + }
>> > +}
>>
>> This looks very similar to drm_gem_fb_prepare_fb(), couldn't you use that
>> function instead ?
>
> Description of drm_gem_fb_prepare_fb() function states that it can be
> used as the &drm_plane_helper_funcs.prepare_fb hook. But we have
> our own hook function which is called rcar_du_vsp_plane_prepare_fb().
> Therefore, I was not sure if it is correct to use drm_gem_fb_prepare_fb()
> inside our hook function.
>
> I will use it in the second version nevertheless.
>
>>
>> > static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
>> > struct drm_plane_state *state)
>> > {
>> > @@ -237,6 +255,8 @@ static int rcar_du_vsp_plane_prepare_fb(struct
>> drm_plane
>> > *plane, }
>> > }
>> >
>> > + rcar_du_vsp_set_fence_for_plane(state);
>> > +
>> > return 0;
>> >
>> > fail:
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>>
>
> Best Regards,
>
> Emre Ucan
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] drm: rcar-du: track dma-buf fences
2018-04-03 18:53 ` Laurent Pinchart
2018-04-03 20:25 ` Daniel Vetter
2018-04-04 10:59 ` Ucan, Emre (ADITG/ESB)
@ 2018-04-05 6:39 ` Ucan, Emre (ADITG/ESB)
2 siblings, 0 replies; 6+ messages in thread
From: Ucan, Emre (ADITG/ESB) @ 2018-04-05 6:39 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: dri-devel
Hello Laurent,
Thank you for your review.
Actually I sent this email yesterday. But I don't see it in mailing list archives because of some reason.
> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Dienstag, 3. April 2018 20:53
> To: Ucan, Emre (ADITG/ESB)
> Cc: dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH] drm: rcar-du: track dma-buf fences
>
> Hello Emre,
>
> Thank you for the patch.
>
> On Tuesday, 3 April 2018 12:14:33 EEST Emre Ucan wrote:
> > We have to check dma-buf reservation objects
> > of our framebuffers before we use them.
> > Otherwise, another driver might be writing
> > on the same buffer which we are using.
> > This would cause visible tearing effects
> > on display.
> >
> > We can use existing atomic helper functions
> > to solve this problem.
> >
> > Signed-off-by: Emre Ucan <eucan@de.adit-jv.com>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 ++
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 20 ++++++++++++++++++++
> > 2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 0329b35..f3da3d1 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -255,6 +255,8 @@ static void rcar_du_atomic_commit_tail(struct
> > drm_atomic_state *old_state) {
> > struct drm_device *dev = old_state->dev;
> >
> > + drm_atomic_helper_wait_for_fences(dev, old_state, false);
> > +
>
> The commit_tail() function in drm_atomic_helper.c, which calls our
> atomic_commit_tail() implementation, already calls
> drm_atomic_helper_wait_for_fences(). Why is there a need to duplicate the
> call
> here ?
You are right. I will remove it in second version.
>
> > /* Apply the atomic update. */
> > drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > drm_atomic_helper_commit_planes(dev, old_state,
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c3..482e23c 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > @@ -18,12 +18,16 @@
> > #include <drm/drm_fb_cma_helper.h>
> > #include <drm/drm_gem_cma_helper.h>
> > #include <drm/drm_plane_helper.h>
> > +#include <drm/drm_atomic.h>
> >
> > #include <linux/bitops.h>
> > #include <linux/dma-mapping.h>
> > +#include <linux/dma-fence.h>
> > +#include <linux/dma-buf.h>
> > #include <linux/of_platform.h>
> > #include <linux/scatterlist.h>
> > #include <linux/videodev2.h>
> > +#include <linux/reservation.h>
> >
> > #include <media/vsp1.h>
> >
> > @@ -203,6 +207,20 @@ static void rcar_du_vsp_plane_setup(struct
> > rcar_du_vsp_plane *plane) plane->index, &cfg);
> > }
> >
> > +static void rcar_du_vsp_set_fence_for_plane(struct drm_plane_state
> *state)
> > +{
> > + struct drm_gem_cma_object *gem;
> > + struct dma_buf *dma_buf;
> > + struct dma_fence *fence;
> > +
> > + gem = drm_fb_cma_get_gem_obj(state->fb, 0);
> > + dma_buf = gem->base.dma_buf;
> > + if (dma_buf) {
> > + fence = reservation_object_get_excl_rcu(dma_buf->resv);
> > + drm_atomic_set_fence_for_plane(state, fence);
>
> Unless I'm mistaken this is used for implicit fencing only. What is your use
> case, wouldn't it be better for userspace to use explicit fencing as that is
> the fence model that has been selected for display ?
We are using Weston on Renesas R-Car H3 SoC. I am using GPU rendered client buffers
directly as scanout buffer for the display. Weston is not using explicit fencing.
>
> > + }
> > +}
>
> This looks very similar to drm_gem_fb_prepare_fb(), couldn't you use that
> function instead ?
Description of drm_gem_fb_prepare_fb() function states that it can be
used as the &drm_plane_helper_funcs.prepare_fb hook. But we have
our own hook function which is called rcar_du_vsp_plane_prepare_fb().
Therefore, I was not sure if it is correct to use drm_gem_fb_prepare_fb()
inside our hook function.
I will use it in the second version nevertheless.
>
> > static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> > struct drm_plane_state *state)
> > {
> > @@ -237,6 +255,8 @@ static int rcar_du_vsp_plane_prepare_fb(struct
> drm_plane
> > *plane, }
> > }
> >
> > + rcar_du_vsp_set_fence_for_plane(state);
> > +
> > return 0;
> >
> > fail:
>
> --
> Regards,
>
> Laurent Pinchart
>
>
Best Regards,
Emre Ucan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread