On Tue, Jun 16, 2020 at 3:57 PM Emil Velikov emil.l.velikov@gmail.com wrote:
On Tue, 16 Jun 2020 at 07:50, Daniel Vetter daniel.vetter@ffwll.ch wrote:
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?
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