| Message ID | 20180330141138.28987-8-daniels@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 30, 2018 at 03:11:22PM +0100, Daniel Stone wrote: > A FB with no object is something we should be shouting very loudly > about, not quietly logging as debug. > > Signed-off-by: Daniel Stone <daniels@collabora.com> > Cc: CK Hu <ck.hu@mediatek.com> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > index 2f4b0ffee598..ac010365d88b 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -95,10 +95,7 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > if (!fb) > return 0; > > - if (!mtk_fb_get_gem_obj(fb)) { > - DRM_DEBUG_KMS("buffer is null\n"); > - return -EFAULT; > - } > + WARN_ON(!mtk_fb_get_gem_obj(fb)); We should presumably still bail out with an error, no? > > if (!state->crtc) > return 0; > -- > 2.16.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, May 17, 2018 at 09:58:19AM -0400, Sean Paul wrote: > On Fri, Mar 30, 2018 at 03:11:22PM +0100, Daniel Stone wrote: > > A FB with no object is something we should be shouting very loudly > > about, not quietly logging as debug. > > > > Signed-off-by: Daniel Stone <daniels@collabora.com> > > Cc: CK Hu <ck.hu@mediatek.com> > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > index 2f4b0ffee598..ac010365d88b 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > @@ -95,10 +95,7 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > > if (!fb) > > return 0; > > > > - if (!mtk_fb_get_gem_obj(fb)) { > > - DRM_DEBUG_KMS("buffer is null\n"); > > - return -EFAULT; > > - } > > + WARN_ON(!mtk_fb_get_gem_obj(fb)); > > We should presumably still bail out with an error, no? I think we should just remove this WARN_ON(). Under what circumstances would this case even happen? If the GEM object for a framebuffer doesn't exist, then mtk_drm_mode_fb_create() will fail and no pointer to struct drm_framebuffer will ever be returned. After that, the GEM object is guaranteed to be there, so the above is effectively dead code. Thierry
On Thu, 2018-05-17 at 16:55 +0200, Thierry Reding wrote: > On Thu, May 17, 2018 at 09:58:19AM -0400, Sean Paul wrote: > > On Fri, Mar 30, 2018 at 03:11:22PM +0100, Daniel Stone wrote: > > > A FB with no object is something we should be shouting very loudly > > > about, not quietly logging as debug. > > > > > > Signed-off-by: Daniel Stone <daniels@collabora.com> > > > Cc: CK Hu <ck.hu@mediatek.com> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > > --- > > > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 5 +---- > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > > index 2f4b0ffee598..ac010365d88b 100644 > > > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > > @@ -95,10 +95,7 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, > > > if (!fb) > > > return 0; > > > > > > - if (!mtk_fb_get_gem_obj(fb)) { > > > - DRM_DEBUG_KMS("buffer is null\n"); > > > - return -EFAULT; > > > - } > > > + WARN_ON(!mtk_fb_get_gem_obj(fb)); > > > > We should presumably still bail out with an error, no? > > I think we should just remove this WARN_ON(). Under what circumstances > would this case even happen? If the GEM object for a framebuffer doesn't > exist, then mtk_drm_mode_fb_create() will fail and no pointer to struct > drm_framebuffer will ever be returned. After that, the GEM object is > guaranteed to be there, so the above is effectively dead code. > > Thierry I agree with Thierry. The case should not happen. So just remove this checking. Regards, CK > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c index 2f4b0ffee598..ac010365d88b 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c @@ -95,10 +95,7 @@ static int mtk_plane_atomic_check(struct drm_plane *plane, if (!fb) return 0; - if (!mtk_fb_get_gem_obj(fb)) { - DRM_DEBUG_KMS("buffer is null\n"); - return -EFAULT; - } + WARN_ON(!mtk_fb_get_gem_obj(fb)); if (!state->crtc) return 0;
A FB with no object is something we should be shouting very loudly about, not quietly logging as debug. Signed-off-by: Daniel Stone <daniels@collabora.com> Cc: CK Hu <ck.hu@mediatek.com> Cc: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/gpu/drm/mediatek/mtk_drm_plane.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)