All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 14/22] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active
Date: Thu, 15 Dec 2016 16:56:15 +0200	[thread overview]
Message-ID: <87f5c29f-e4f2-ef96-10cd-e7d3e04fb04d@ti.com> (raw)
In-Reply-To: <2580087.D9jCYKYxOH@avalon>


[-- Attachment #1.1.1: Type: text/plain, Size: 1177 bytes --]

On 15/12/16 16:51, Laurent Pinchart wrote:

>>> +	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>>
>> I don't like this style very much. I think WARN()s should be without
>> side effects.
>>
>> Also, WARN only gives benefit when we don't know what the call stack is.
>> Afaik, there's only one way omap_crtc_atomic_flush can be called, so
>> it's just extra spam and dev_err or dev_warn should be enough.
> 
> I've used it because the equivalent statements testing omap_crtc-
>> vblank_irq.registered used WARN_ON() too. WARN_ON() is also a bit more vocal, 
> it really gets the point across. As the function really should not return an 
> error unless in case of a driver bug, I don't think it will generate any spam. 
> I don't care too much though, I can replace it with a dev_err() if you insist.

I don't mind using WARN_ON() that much. But that's the comment I've
received a few times, so I shared it =).

What I do care is the side effect inside WARN_ON. At least for me it's
quite easy to miss that it's actually having a side effect, as I expect
WARN_ON() to be just a check, like assert(). So when reading the code, I
skip the WARN_ONs.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2016-12-15 14:56 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-14  0:27 [PATCH v4 00/22] OMAP DRM fixes and improvements Laurent Pinchart
2016-12-14  0:27 ` [PATCH v4 01/22] drm: omapdrm: fb: Limit number of planes per framebuffer to two Laurent Pinchart
2016-12-14  0:27 ` [PATCH v4 02/22] drm: omapdrm: fb: Use format information provided by the DRM core Laurent Pinchart
2016-12-14 10:07   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 03/22] drm: omapdrm: fb: Simplify objects lookup when creating framebuffer Laurent Pinchart
2016-12-14  0:27 ` [PATCH v4 04/22] drm: omapdrm: fb: Simplify mode command checks " Laurent Pinchart
2016-12-14  0:27 ` [PATCH v4 05/22] drm: omapdrm: fb: Turn framebuffer creation error messages into debug Laurent Pinchart
2016-12-14  0:27 ` [PATCH v4 06/22] drm: omapdrm: Handle FIFO underflow IRQs internally Laurent Pinchart
2016-12-14 10:22   ` Tomi Valkeinen
2016-12-14 11:48     ` Laurent Pinchart
2016-12-14 13:13       ` Tomi Valkeinen
2016-12-15  9:02       ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 07/22] drm: omapdrm: Handle CRTC error IRQs directly Laurent Pinchart
2016-12-14  0:27 ` [PATCH v4 08/22] drm: omapdrm: Handle OCP error IRQ directly Laurent Pinchart
2016-12-14 10:24   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 09/22] drm: omapdrm: Replace DSS manager state check with omapdrm CRTC state Laurent Pinchart
2016-12-14 10:36   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 10/22] drm: omapdrm: Let the DRM core skip plane commit on inactive CRTCs Laurent Pinchart
2016-12-14 10:43   ` Tomi Valkeinen
2016-12-14 11:26     ` Laurent Pinchart
2016-12-14  0:27 ` [PATCH v4 11/22] drm: omapdrm: Check the CRTC software state at enable/disable time Laurent Pinchart
2016-12-14 14:54   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 12/22] drm: omapdrm: Prevent processing the same event multiple times Laurent Pinchart
2016-12-15 12:20   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 13/22] drm: omapdrm: Use a spinlock to protect the CRTC pending flag Laurent Pinchart
2016-12-14  0:27 ` [PATCH v4 14/22] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active Laurent Pinchart
2016-12-15 12:52   ` Tomi Valkeinen
2016-12-15 14:51     ` Laurent Pinchart
2016-12-15 14:56       ` Tomi Valkeinen [this message]
2016-12-18  2:12   ` [PATCH v4.1 " Laurent Pinchart
2016-12-19  9:06     ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 15/22] drm: omapdrm: Don't expose the omap_irq_(un)register() functions Laurent Pinchart
2016-12-15 12:56   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 16/22] drm: omapdrm: Remove unused parameter from omap_drm_irq handler Laurent Pinchart
2016-12-15 12:57   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 17/22] drm: omapdrm: Don't call DISPC power handling in IRQ wait functions Laurent Pinchart
2016-12-15 13:00   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 18/22] drm: omapdrm: Inline the pipe2vbl function Laurent Pinchart
2016-12-14 10:25   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 19/22] drm: omapdrm: Simplify IRQ wait implementation Laurent Pinchart
2016-12-16 12:24   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 20/22] drm: omapdrm: Remove global variables Laurent Pinchart
2016-12-16 12:31   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 21/22] drm: omapdrm: Use sizeof(*var) instead of sizeof(type) for structures Laurent Pinchart
2016-12-15 13:02   ` Tomi Valkeinen
2016-12-14  0:27 ` [PATCH v4 22/22] drm: omapdrm: Perform initialization/cleanup at probe/remove time Laurent Pinchart
2016-12-16 12:44   ` Tomi Valkeinen
2016-12-16 13:54     ` Laurent Pinchart
2016-12-19  9:15   ` [PATCH v4.1 " Laurent Pinchart
2016-12-19  9:25     ` Tomi Valkeinen
2016-12-14  8:48 ` [PATCH v4 00/22] OMAP DRM fixes and improvements Tomi Valkeinen
2016-12-14 11:50   ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87f5c29f-e4f2-ef96-10cd-e7d3e04fb04d@ti.com \
    --to=tomi.valkeinen@ti.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=laurent.pinchart@ideasonboard.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.