Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: David Lechner <david@lechnology.com>,
	David Airlie <airlied@linux.ie>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled
Date: Tue, 16 Jun 2020 08:50:06 +0200	[thread overview]
Message-ID: <CAKMK7uEEkH+8BuFcFUVTv6p8swZZTwcho-HNx5GdZTO1vHDoeg@mail.gmail.com> (raw)
In-Reply-To: <CACvgo51AVVOxhGK2Uin=ZLgPpihJiEsnc6pvAyKqFKPvLdOzLA@mail.gmail.com>

On Mon, Jun 15, 2020 at 11:35 PM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Hi Daniel,
>
> On Fri, 12 Jun 2020 at 17:01, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > The atomic helpers try really hard to not lose track of things,
> > duplicating enabled tracking in the driver is at best confusing.
> > Double-enabling or disabling is a bug in atomic helpers.
> >
> > In the fb_dirty function we can just assume that the fb always exists,
> > simple display pipe helpers guarantee that the crtc is only enabled
> > together with the output, so we always have a primary plane around.
> >
> > Now in the update function we need to be a notch more careful, since
> > that can also get called when the crtc is off. And we don't want to
> > upload frames when that's the case, so filter that out too.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: David Lechner <david@lechnology.com>
> > ---
> >  drivers/gpu/drm/drm_mipi_dbi.c | 16 ++++++----------
> >  drivers/gpu/drm/tiny/ili9225.c | 12 +++---------
> >  drivers/gpu/drm/tiny/st7586.c  | 11 +++--------
> >  include/drm/drm_mipi_dbi.h     |  5 -----
> >  4 files changed, 12 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> > index fd8d672972a9..79532b9a324a 100644
> > --- a/drivers/gpu/drm/drm_mipi_dbi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> > @@ -268,7 +268,7 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> >         bool full;
> >         void *tr;
> >
> > -       if (!dbidev->enabled)
> > +       if (WARN_ON(!fb))
> >                 return;
> >
> AFAICT no other driver has such WARN_ON. Let's drop that - it is
> pretty confusing and misleading as-is.

Yeah, this is a helper library which might be used wrongly by drivers.
That's why I put it in - if you don't put all the various calls
together correctly, this should at least catch one case. So really
would like to keep this, can I convince you?
-Daniel

> With that, patches 7/8 and 8/8 are:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>
> -Emil


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel