From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gurchetan Singh Subject: Re: [PATCH 2/2] drm: clean up internally created framebuffer on CRTC disable Date: Fri, 8 Dec 2017 14:17:39 -0800 Message-ID: References: <20171207030159.3007-1-gurchetansingh@chromium.org> <20171207091648.iyxv7hzznpswgenh@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1697788437==" Return-path: Received: from mail-lf0-x244.google.com (mail-lf0-x244.google.com [IPv6:2a00:1450:4010:c07::244]) by gabe.freedesktop.org (Postfix) with ESMTPS id D0257898AF for ; Fri, 8 Dec 2017 22:17:43 +0000 (UTC) Received: by mail-lf0-x244.google.com with SMTP id 94so13308215lfy.10 for ; Fri, 08 Dec 2017 14:17:43 -0800 (PST) Received: from mail-lf0-f53.google.com (mail-lf0-f53.google.com. [209.85.215.53]) by smtp.gmail.com with ESMTPSA id a15sm1662049ljb.11.2017.12.08.14.17.40 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Dec 2017 14:17:41 -0800 (PST) Received: by mail-lf0-f53.google.com with SMTP id j124so13339493lfg.2 for ; Fri, 08 Dec 2017 14:17:40 -0800 (PST) In-Reply-To: <20171207091648.iyxv7hzznpswgenh@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: ML dri-devel List-Id: dri-devel@lists.freedesktop.org --===============1697788437== Content-Type: multipart/alternative; boundary="94eb2c1c18c283d57e055fdb8df4" --94eb2c1c18c283d57e055fdb8df4 Content-Type: text/plain; charset="UTF-8" The problem I'm trying to solve is that the internal cursor fb is leaky and even present after closing the drm driver fd. This can be seen by running modetest after this test case: https://chromium.googlesource.com/chromiumos/platform/drm-tests/+/master/drm_cursor_test.c However, as you mentioned, the approach in this patch is not consistent with the uapi. Adding the internal fb to file_priv->fbs is another approach, though I haven't gotten it to work in 100% of all cases. What do you recommend (if anything at all)? On Thu, Dec 7, 2017 at 1:16 AM, Daniel Vetter wrote: > On Wed, Dec 06, 2017 at 07:01:59PM -0800, Gurchetan Singh wrote: > > When a CRTC is disabled and we used an internally created framebuffer, > > this patch disables the cursor plane and drops the reference that was > > introduced when we called drm_internal_framebuffer_create. > > > > Signed-off-by: Gurchetan Singh > > What kind of bug are you trying to fix here? plane and cursor uapi is that > when you re-enable the crtc, all the planes will be there again. Only > exception is the primary plane, but that's only because set_config both > disables the crtc _and_ the primary plane. > > Without more detail of what's going on I have no idea what exactly you're > trying to achieve here. > -Daniel > > --- > > drivers/gpu/drm/drm_crtc.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index f0556e654116..d732cca4879f 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -101,12 +101,19 @@ EXPORT_SYMBOL(drm_crtc_from_index); > > */ > > int drm_crtc_force_disable(struct drm_crtc *crtc) > > { > > + struct drm_framebuffer *fb; > > struct drm_mode_set set = { > > .crtc = crtc, > > }; > > > > WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev)); > > > > + if (crtc->cursor && crtc->cursor->fb && > crtc->cursor->fb->internal) { > > + fb = crtc->cursor->fb; > > + drm_plane_force_disable(crtc->cursor); > > + drm_framebuffer_unreference(fb); > > + } > > + > > return drm_mode_set_config_internal(&set); > > } > > EXPORT_SYMBOL(drm_crtc_force_disable); > > -- > > 2.13.5 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > --94eb2c1c18c283d57e055fdb8df4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
The problem I'm trying to solve is that the internal c= ursor fb is leaky and even present after closing the drm driver fd.=C2=A0 T= his can be seen by running modetest after this test case:

https://chromium.googlesource.com/chromiumos/= platform/drm-tests/+/master/drm_cursor_test.c

<= div>However, as you mentioned, the approach in this patch is not consistent= with the uapi.=C2=A0 Adding the internal fb to=C2=A0file_priv->fbs is a= nother approach, though I haven't gotten it to work in 100% of all case= s.=C2=A0 What do you recommend (if anything at all)?

On Thu, Dec 7, 2017 at 1:16= AM, Daniel Vetter <daniel@ffwll.ch> wrote:
On Wed, Dec 06, 2017 at 07:01:59PM -0800, = Gurchetan Singh wrote:
> When a CRTC is disabled and we used an internally created framebuffer,=
> this patch disables the cursor plane and drops the reference that was<= br> > introduced when we called drm_internal_framebuffer_create.
>
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>

What kind of bug are you trying to fix here? plane and cursor uapi i= s that
when you re-enable the crtc, all the planes will be there again. Only
exception is the primary plane, but that's only because set_config both=
disables the crtc _and_ the primary plane.

Without more detail of what's going on I have no idea what exactly you&= #39;re
trying to achieve here.
-Daniel
> ---
>=C2=A0 drivers/gpu/drm/drm_crtc.c | 7 +++++++
>=C2=A0 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index f0556e654116..d732cca4879f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -101,12 +101,19 @@ EXPORT_SYMBOL(drm_crtc_from_index);
>=C2=A0 =C2=A0*/
>=C2=A0 int drm_crtc_force_disable(struct drm_crtc *crtc)
>=C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0struct drm_framebuffer *fb;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_mode_set set =3D {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.crtc =3D crtc,<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0};
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0WARN_ON(drm_drv_uses_atomic_modeset(crt= c->dev));
>
> +=C2=A0 =C2=A0 =C2=A0if (crtc->cursor && crtc->cursor-&g= t;fb && crtc->cursor->fb->internal) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fb =3D crtc->curso= r->fb;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0drm_plane_force_disab= le(crtc->cursor);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0drm_framebuffer_unref= erence(fb);
> +=C2=A0 =C2=A0 =C2=A0}
> +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return drm_mode_set_config_internal(&set);
>=C2=A0 }
>=C2=A0 EXPORT_SYMBOL(drm_crtc_force_disable);
> --
> 2.13.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.fre= edesktop.org
> https://lists.freedesktop.org/mail= man/listinfo/dri-devel

--
Daniel Vetter
Software Engineer, Intel Corporation
http:= //blog.ffwll.ch

--94eb2c1c18c283d57e055fdb8df4-- --===============1697788437== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1697788437==--