RE: [PATCH v2] drm: rcar-du: track dma-buf fences
Hi Laurent, (snip) > > I will send a new patch with these changes after I tested it. Is it possible > > that improved patch lands on 4.17 and 4.14 stable versions? What is the > > process to propose a patch to a stable release ? > > The process is pretty simple, it's documented in > Documentation/process/stable- > kernel-rules.rst. The file states the following requirement for a patch to be > eligible for stable backporting: > > - It must fix a problem that causes a build error (but not for things >marked CONFIG_BROKEN), an oops, a hang, data corruption, a real >security issue, or some "oh, that's not good" issue. In short, something >critical > > I'll let you decide whether this patch qualifies, as it could be argued that > it implements a new feature. > > -- > Regards, > > Laurent Pinchart > > IMHO, it is not a new feature, but It is a bug. Because explicit fencing is a recent kernel feature. As Daniel Vetter stated, most X and wayland setups still rely on implicit fencing. Anyway this patch must be integrated first to Linus' tree, before I can propose to the stable release. It would be great if it can be part of 4.17. Best Regards, Emre Ucan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v2] drm: rcar-du: track dma-buf fences
Hi Laurent, Sorry for late reply. > -Original Message- > From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] > Sent: Montag, 23. April 2018 23:47 > To: Ucan, Emre (ADITG/ESB) > Cc: dri-devel@lists.freedesktop.org > Subject: Re: [PATCH v2] drm: rcar-du: track dma-buf fences > > Hi Emre, > > Sorry for the late reply. > > On Wednesday, 4 April 2018 14:03:57 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. > > I've always found the 72 columns limit in commit messages to be quite > limiting, especially for the subject line. Out of curiosity, is there any > specific reason why you limit it further ? I tried to keep lines short. But I did not check if they are shorter than 72. I will improve it. > > > v2 changes: > > - Remove drm_atomic_helper_wait_for_fences() > > call in rcar_du_kms.c. The commit_tail() > > function in drm_atomic_helper.c, which calls > > our atomic_commit_tail() implementation, > > already calls it. > > - Remove proposed rcar_du_vsp_set_fence_for_plane() > > function. Call drm_gem_fb_prepare_fb(), which > > calls drm_atomic_set_fence_for_plane(). > > > > Signed-off-by: Emre Ucan <eu...@de.adit-jv.com> > > --- > > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c3..fbad616 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > @@ -18,6 +18,7 @@ > > #include > > #include > > #include > > +#include > > Nitpicking, could you please keep the headers alphabetically sorted ? It helps > locating duplicates quickly. No problem. I will fix it. > > > #include > > #include > > @@ -237,7 +238,7 @@ static int rcar_du_vsp_plane_prepare_fb(struct > drm_plane > > *plane, } > > } > > > > - return 0; > > + return drm_gem_fb_prepare_fb(plane, state); > > If drm_gem_fb_prepare_fb() fails we need to clean up the operations > performed > earlier in this function. I think the following would be enough. > > ret = drm_gem_fb_prepare_fb(plane, state); > if (ret) > goto fail; > > return 0; > > Could you please test this ? No problem. > > > fail: > > while (i--) { > > -- > Regards, > > Laurent Pinchart > > I will send a new patch with these changes after I tested it. Is it possible that improved patch lands on 4.17 and 4.14 stable versions? What is the process to propose a patch to a stable release ? Best Regards, Emre Ucan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH] drm: rcar-du: track dma-buf fences
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.pinch...@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 <eu...@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 > > #include > > #include > > +#include > > > > #include > > #include > > +#include > > +#include > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -203,6 +207,20 @@ static void rcar_du_vsp_plane_setup(struct > > rcar_du_vsp_plane *plane) plane->index, ); > > } > > > > +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 _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
RE: [PATCH] drm: rcar-du: track dma-buf fences
Hello Laurent Thank you for your review. > -Original Message- > From: Laurent Pinchart [mailto:laurent.pinch...@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 <eu...@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 > > #include > > #include > > +#include > > > > #include > > #include > > +#include > > +#include > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -203,6 +207,20 @@ static void rcar_du_vsp_plane_setup(struct > > rcar_du_vsp_plane *plane) plane->index, ); > > } > > > > +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 _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