| Message ID | 1414501814-1465-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote: > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 8b80d68..f9ddedc 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs) > return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal); > } > > +/** > + * intel_pipe_update_start() - start update of a set of display registers > + * @crtc: the crtc of which the registers are going to be updated > + * @start_vbl_count: vblank counter return pointer used for error checking > + * > + * Mark the start of an update to pipe registers that should be updated > + * atomically regarding vblank. If the next vblank will happens within > + * the next 100 us, this function waits until the vblank passes. > + * > + * After a successful call to this function, interrupts will be disabled > + * until a subsequent call to intel_pipe_update_end(). That is done to > + * avoid random delays. The value written to @start_vbl_count should be > + * supplied to intel_pipe_update_end() for error checking. > + * > + * Return: true if the call was successful > + */ It's nice that people now go overboard with kerneldoc, but I think we need to strike a good balance. And in general I think documenting static inline functions isn't worth it - they really should be self-explanatory as-is. Documentation is imo only really useful for the bigger stuff, which usually means it's used in a few places all over. So non-static functions. This one here is a bit a corner-case since it's more of a generic piece of infrastructure. But otoh I think in the end we'll have exactly one caller of these functions for all atomic plane updates we'll need. Everyone else (legacy code, modeset code, ...) will just call that higher-level function. Not sure what to do here. -Daniel
On 11/03/2014 02:33 PM, Daniel Vetter wrote: > On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote: >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index 8b80d68..f9ddedc 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs) >> return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal); >> } >> >> +/** >> + * intel_pipe_update_start() - start update of a set of display registers >> + * @crtc: the crtc of which the registers are going to be updated >> + * @start_vbl_count: vblank counter return pointer used for error checking >> + * >> + * Mark the start of an update to pipe registers that should be updated >> + * atomically regarding vblank. If the next vblank will happens within >> + * the next 100 us, this function waits until the vblank passes. >> + * >> + * After a successful call to this function, interrupts will be disabled >> + * until a subsequent call to intel_pipe_update_end(). That is done to >> + * avoid random delays. The value written to @start_vbl_count should be >> + * supplied to intel_pipe_update_end() for error checking. >> + * >> + * Return: true if the call was successful >> + */ > > It's nice that people now go overboard with kerneldoc, but I think we need > to strike a good balance. And in general I think documenting static inline > functions isn't worth it - they really should be self-explanatory as-is. > > Documentation is imo only really useful for the bigger stuff, which > usually means it's used in a few places all over. So non-static functions. > > This one here is a bit a corner-case since it's more of a generic piece of > infrastructure. But otoh I think in the end we'll have exactly one caller > of these functions for all atomic plane updates we'll need. Everyone else > (legacy code, modeset code, ...) will just call that higher-level > function. Not sure what to do here. s/**/*/ ? Cheers, Ander --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
2014-11-03 10:33 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote: >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index 8b80d68..f9ddedc 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs) >> return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal); >> } >> >> +/** >> + * intel_pipe_update_start() - start update of a set of display registers >> + * @crtc: the crtc of which the registers are going to be updated >> + * @start_vbl_count: vblank counter return pointer used for error checking >> + * >> + * Mark the start of an update to pipe registers that should be updated >> + * atomically regarding vblank. If the next vblank will happens within >> + * the next 100 us, this function waits until the vblank passes. >> + * >> + * After a successful call to this function, interrupts will be disabled >> + * until a subsequent call to intel_pipe_update_end(). That is done to >> + * avoid random delays. The value written to @start_vbl_count should be >> + * supplied to intel_pipe_update_end() for error checking. >> + * >> + * Return: true if the call was successful >> + */ > > It's nice that people now go overboard with kerneldoc, but I think we need > to strike a good balance. And in general I think documenting static inline > functions isn't worth it - they really should be self-explanatory as-is. But patch 3 exports these functions and uses them from another file. > > Documentation is imo only really useful for the bigger stuff, which > usually means it's used in a few places all over. So non-static functions. The comments he introduced are useful and helped me review patch 3 without having to look at the function implementation and waste 20 minutes wondering what it was supposed to do. To me, this patch is an improvement to the codebase, so with or without the extra '*' chars: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > This one here is a bit a corner-case since it's more of a generic piece of > infrastructure. But otoh I think in the end we'll have exactly one caller > of these functions for all atomic plane updates we'll need. Everyone else > (legacy code, modeset code, ...) will just call that higher-level > function. Not sure what to do here. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Nov 03, 2014 at 03:26:36PM -0200, Paulo Zanoni wrote: > 2014-11-03 10:33 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > > On Tue, Oct 28, 2014 at 03:10:12PM +0200, Ander Conselvan de Oliveira wrote: > >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++ > >> 1 file changed, 25 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > >> index 8b80d68..f9ddedc 100644 > >> --- a/drivers/gpu/drm/i915/intel_sprite.c > >> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >> @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs) > >> return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal); > >> } > >> > >> +/** > >> + * intel_pipe_update_start() - start update of a set of display registers > >> + * @crtc: the crtc of which the registers are going to be updated > >> + * @start_vbl_count: vblank counter return pointer used for error checking > >> + * > >> + * Mark the start of an update to pipe registers that should be updated > >> + * atomically regarding vblank. If the next vblank will happens within > >> + * the next 100 us, this function waits until the vblank passes. > >> + * > >> + * After a successful call to this function, interrupts will be disabled > >> + * until a subsequent call to intel_pipe_update_end(). That is done to > >> + * avoid random delays. The value written to @start_vbl_count should be > >> + * supplied to intel_pipe_update_end() for error checking. > >> + * > >> + * Return: true if the call was successful > >> + */ > > > > It's nice that people now go overboard with kerneldoc, but I think we need > > to strike a good balance. And in general I think documenting static inline > > functions isn't worth it - they really should be self-explanatory as-is. > > But patch 3 exports these functions and uses them from another file. Ah, I've missed that, comment retracted ... > > > > > Documentation is imo only really useful for the bigger stuff, which > > usually means it's used in a few places all over. So non-static functions. > > The comments he introduced are useful and helped me review patch 3 > without having to look at the function implementation and waste 20 > minutes wondering what it was supposed to do. > > To me, this patch is an improvement to the codebase, so with or > without the extra '*' chars: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> ... and patch merged. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 8b80d68..f9ddedc 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -46,6 +46,22 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs) return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal); } +/** + * intel_pipe_update_start() - start update of a set of display registers + * @crtc: the crtc of which the registers are going to be updated + * @start_vbl_count: vblank counter return pointer used for error checking + * + * Mark the start of an update to pipe registers that should be updated + * atomically regarding vblank. If the next vblank will happens within + * the next 100 us, this function waits until the vblank passes. + * + * After a successful call to this function, interrupts will be disabled + * until a subsequent call to intel_pipe_update_end(). That is done to + * avoid random delays. The value written to @start_vbl_count should be + * supplied to intel_pipe_update_end() for error checking. + * + * Return: true if the call was successful + */ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count) { struct drm_device *dev = crtc->base.dev; @@ -112,6 +128,15 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl return true; } +/** + * intel_pipe_update_end() - end update of a set of display registers + * @crtc: the crtc of which the registers were updated + * @start_vbl_count: start vblank counter (used for error checking) + * + * Mark the end of an update started with intel_pipe_update_start(). This + * re-enables interrupts and verifies the update was actually completed + * before a vblank using the value of @start_vbl_count. + */ static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count) { struct drm_device *dev = crtc->base.dev;
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)