On Tue, Jun 16, 2020 at 3:57 PM Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote: > > On Tue, 16 Jun 2020 at 07:50, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: > > > > On Mon, Jun 15, 2020 at 11:35 PM Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote: > > > > > > Hi Daniel, > > > > > > On Fri, 12 Jun 2020 at 17:01, Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxxx> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > > > Cc: Maxime Ripard <mripard@xxxxxxxxxx> > > > > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> > > > > Cc: David Airlie <airlied@xxxxxxxx> > > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > > > Cc: David Lechner <david@xxxxxxxxxxxxxx> > > > > --- > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
