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

[Daniel Vetter]

On Tue, Jun 16, 2020 at 3:57 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> On Tue, 16 Jun 2020 at 07:50, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> >
> > On Mon, Jun 15, 2020 at 11:35 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Fri, 12 Jun 2020 at 17:01, Daniel Vetter <daniel.vetter at 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 at intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > Cc: Maxime Ripard <mripard at kernel.org>
> > > > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > > > Cc: David Airlie <airlied at linux.ie>
> > > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > > Cc: David Lechner <david at 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?
>
> There are plenty of similar places where a drm library/helper can be
> misused, lacking a WARN. Nevertheless - sure feel free to keep it.

Yeah I agree, we can't check for everything. Personally I think a
check is warranted in two conditions:
- drivers got it wrong, and the WARNING helps catch driver-bugs we've
seen in the wild. Not really the case here
- drivers do check something as defensive programming, but it's an
invariant enforced by higher levels or helpers. Those I like to
convert to WARNING so that other driver authors learn that this should
never happen. This is such a case imo, I removed a bunch of fb checks
from drivers here.

But yeah I think we should only add WARNING checks if this is actually
something people have gotten wrong, otherwise there's just too many of
them, distracting from the code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch