Re: [Intel-gfx] [PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

[Daniel Kurtz]

Hi Ville,

Two fixes inline...

On Wed, Jul 27, 2016 at 1:34 AM,  wrote:
>
> From: Ville Syrjälä 
>
> Add a version of drm_plane_helper_check_update() which takes a plane
> state instead of having the caller pass in everything.
>
> And to reduce code duplication, let's reimplement
> drm_plane_helper_check_update() in terms of the new function, by
> having a tempororary plane state on the stack.
>
> v2: Add a note that the functions modifies the state (Chris)
>
> Cc: Chris Wilson 
> Signed-off-by: Ville Syrjälä 

[snip...]

> +int drm_plane_helper_check_update(struct drm_plane *plane,
> + struct drm_crtc *crtc,
> + struct drm_framebuffer *fb,
> + struct drm_rect *src,
> + struct drm_rect *dst,
> + const struct drm_rect *clip,
> + unsigned int rotation,
> + int min_scale,
> + int max_scale,
> + bool can_position,
> + bool can_update_disabled,
> + bool *visible)
> +{
> +   struct drm_plane_state state = {
> +   .plane = plane,
> +   .crtc = crtc,
> +   .fb = fb,
> +   .src_x = src->x1,
> +   .src_y = src->x2,

This should be:
src->y1

> +   .src_w = drm_rect_width(src),
> +   .src_h = drm_rect_height(src),
> +   .crtc_x = dst->x1,
> +   .crtc_y = dst->x2,

And this should be:
dst->y1

Thanks,
-Dan
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH xf96-video-intel] DRI2GetMSC: Do not send a bogus ust when no drawable is not displayed

[Daniel Kurtz]

According to the opengl glx_sync_control spec, the Unadjusted System Time
(or UST) is a 64-bit monotonically increasing counter that is available
throughout the system:
http://www.opengl.org/registry/specs/OML/glx_sync_control.txt

Therefore, sending 0, even in this corner case, is out of spec.
Instead, just return FALSE indicating that the operation could not be
completed.

Signed-off-by: Daniel Kurtz djku...@chromium.org
---
 src/intel_dri.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/intel_dri.c b/src/intel_dri.c
index f351203..179bfb7 100644
--- a/src/intel_dri.c
+++ b/src/intel_dri.c
@@ -1339,12 +1339,9 @@ I830DRI2GetMSC(DrawablePtr draw, CARD64 *ust, CARD64 
*msc)
drmVBlank vbl;
int ret, pipe = I830DRI2DrawablePipe(draw);
 
-   /* Drawable not displayed, make up a value */
-   if (pipe == -1) {
-   *ust = 0;
-   *msc = 0;
-   return TRUE;
-   }
+   /* Drawable not displayed, return FALSE */
+   if (pipe == -1)
+   return FALSE;
 
vbl.request.type = DRM_VBLANK_RELATIVE | pipe_select(pipe);
vbl.request.sequence = 0;
-- 
1.8.1.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/8] drm/i915: use the gmbus irq for waits

[Daniel Kurtz]

On Sun, Sep 9, 2012 at 5:00 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:

 We need two special things to properly wire this up:
 - Add another argument to gmbus_wait_hw_status to pass in the
   correct interrupt bit in gmbus4.
 - Since we can only get an irq for one of the two events we want,
   hand-roll the wait_event_timeout code so that we wake up every
   jiffie and can check for NAKs. This way we also subsume gmbus
   support for platforms without interrupts (or where those are not
   yet enabled).


Hi Daniel V,


 The important bit really is to only enable one gmbus interrupt source
 at the same time - with that piece of lore figured out, this seems to
 work flawlessly.

Great find!
Overall, this looks and sounds great.
See some comments inline...

 Ben Widawsky rightfully complained the lack of measurements for the
 claimed benefits (especially since the first version was actually
 broken and fell back to bit-banging). Previously reading the 256 byte
 hdmi EDID takes about 72 ms here. With this patch it's down to 33 ms.
 Given that transfering the 256 bytes over i2c at wire speed takes
 20.5ms alone, the reduction in additional overhead is rather nice.

 v2: Chris Wilson wondered whether GMBUS4 might contain some set bits
 when booting up an hence result in some spurious interrupts. Since we
 clear GMBUS4 after every wait and we do gmbus transfer really early in
 the setup sequence to detect displays the window is small, but still
 be paranoid and clear it properly.

 v3: Clarify the comment that gmbus irq generation can only support one
 kind of event, why it bothers us and how we work around that limit.

 Cc: Daniel Kurtz djku...@chromium.org
 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
  drivers/gpu/drm/i915/i915_irq.c  |  4 
  drivers/gpu/drm/i915/intel_i2c.c | 45 
 ++--
  3 files changed, 40 insertions(+), 11 deletions(-)

 diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
 index 26c6959..13b9e6a 100644
 --- a/drivers/gpu/drm/i915/i915_drv.h
 +++ b/drivers/gpu/drm/i915/i915_drv.h
 @@ -419,6 +419,8 @@ typedef struct drm_i915_private {
  */
 uint32_t gpio_mmio_base;

 +   wait_queue_head_t gmbus_wait_queue;
 +
 struct pci_dev *bridge_dev;
 struct intel_ring_buffer ring[I915_NUM_RINGS];
 uint32_t next_seqno;
 diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
 index 86f1690..1741f2e 100644
 --- a/drivers/gpu/drm/i915/i915_irq.c
 +++ b/drivers/gpu/drm/i915/i915_irq.c
 @@ -598,7 +598,11 @@ out:

  static void gmbus_irq_handler(struct drm_device *dev)
  {
 +   struct drm_i915_private *dev_priv = (drm_i915_private_t *) 
 dev-dev_private;
 +
 DRM_DEBUG_DRIVER(GMBUS interrupt\n);
 +
 +   wake_up_all(dev_priv-gmbus_wait_queue);
  }

  static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 diff --git a/drivers/gpu/drm/i915/intel_i2c.c 
 b/drivers/gpu/drm/i915/intel_i2c.c
 index 57decac..7413595 100644
 --- a/drivers/gpu/drm/i915/intel_i2c.c
 +++ b/drivers/gpu/drm/i915/intel_i2c.c
 @@ -64,6 +64,7 @@ intel_i2c_reset(struct drm_device *dev)
  {
 struct drm_i915_private *dev_priv = dev-dev_private;
 I915_WRITE(dev_priv-gpio_mmio_base + GMBUS0, 0);
 +   I915_WRITE(dev_priv-gpio_mmio_base + GMBUS4, 0);
  }

  static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool 
 enable)
 @@ -205,20 +206,38 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)

  static int
  gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 -u32 gmbus2_status)
 +u32 gmbus2_status,
 +u32 gmbus4_irq_en)
  {
 -   int ret;
 +   int i;
 int reg_offset = dev_priv-gpio_mmio_base;
 -   u32 gmbus2;
 +   u32 gmbus2 = 0;


Technically, initializing gmbus2 here isn't necessary, since you
always assign it first before reading.


 +   DEFINE_WAIT(wait);
 +
 +   /* Important: The hw handles only the first bit, so set only one! 
 Since
 +* we also need to check for NAKs besides the hw ready/idle signal, we
 +* need to wake up periodically and check that ourselves. */


-- It is unfortunate that we can't enable both HW_WAIT/HW_RDY  NAK irqs.
When I tried it before, it seemed to work...  but something was always
unstable, and I never figured out what.


 +   I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en);

 -   ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) 
 -  (GMBUS_SATOER | gmbus2_status),
 -  50);
 +   for (i = 0; i  msecs_to_jiffies(50) + 1; i++) {


Should there be an initial check of our condition before entering the wait?


 +   prepare_to_wait(dev_priv-gmbus_wait_queue, wait,
 +   TASK_UNINTERRUPTIBLE);


Should this wait be interruptible?

 +
 +   gmbus2