Re: [Intel-gfx] [PATCH 08/12] drm/i915: Allow calling intel_adjust_tile_offset() multiple times
On 5/27/2016 1:53 PM, Ville Syrjälä wrote: On Tue, May 03, 2016 at 06:39:57PM +0300, ville.syrj...@linux.intel.com wrote: From: Ville SyrjäläMinimize the resulting X coordinate after intel_adjust_tile_offset() is done with it's offset adjustment. This allows calling intel_adjust_tile_offset() multiple times in case we need to adjust the offset several times. Signed-off-by: Ville Syrjälä I was hoping to push most of this, but this patch still needs to be reviewed before I do that. I don't want to push without the SKL X-tile hack (which needs this). Sivakumar, are you still looking at this or should I turn elsewhere? sorry for the delay in reply, can you check if someone else can review this patch alone ? regards, Sivakumar --- drivers/gpu/drm/i915/intel_display.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 438f3bd86e48..17f9f014e808 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2361,6 +2361,7 @@ static u32 intel_adjust_tile_offset(int *x, int *y, u32 old_offset, u32 new_offset) { + unsigned int pitch_pixels = pitch_tiles * tile_width; unsigned int tiles; WARN_ON(old_offset & (tile_size - 1)); @@ -2372,6 +2373,10 @@ static u32 intel_adjust_tile_offset(int *x, int *y, *y += tiles / pitch_tiles * tile_height; *x += tiles % pitch_tiles * tile_width; + /* minimize x in case it got needlessly big */ + *y += *x / pitch_pixels * tile_height; + *x %= pitch_pixels; + return new_offset; } -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Invoke the DP Compliance test request handler in the short pulse path
i am bit partial :) coz i was involved but isn't the same done in patch shared earlier ? https://patchwork.freedesktop.org/patch/82587/ why not integrate that here if something more is done in the following patches ? regards, Sivakumar On 4/30/2016 6:58 AM, Manasi Navare wrote: HPD Short pulse test requests occur for DP Compliance Link Training and Video Pattern tests.The DP Test request handler needs to be invoked by these tests in the short pulse path in order to support automated DP Compliance tests. Signed-off-by: Manasi Navare--- drivers/gpu/drm/i915/intel_dp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c12c414..19a95ed 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4257,7 +4257,7 @@ intel_dp_short_pulse(struct intel_dp *intel_dp) sink_irq_vector); if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) - DRM_DEBUG_DRIVER("Test request in short pulse not handled\n"); + intel_dp_handle_test_request(intel_dp); if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); } ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Pass around plane_state instead of fb+rotation
Reviewed-by: Sivakumar ThulasimaniOn 5/3/2016 9:09 PM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä intel_compute_tile_offset() and intel_add_fb_offsets() get passed the fb and the rotation. As both of those come from the plane state we can just pass that in instead. For extra consitency pass the plane state to intel_fb_xy_to_linear() as well even though it only really needs the fb. Signed-off-by: Ville Syrjälä Reviewed-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 35 --- drivers/gpu/drm/i915/intel_drv.h | 9 - drivers/gpu/drm/i915/intel_sprite.c | 22 +++--- 3 files changed, 35 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 87b783c56533..b8ca6bc3595c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2314,8 +2314,10 @@ static int intel_fb_pitch(const struct drm_framebuffer *fb, int plane, * with gen2/3, and 90/270 degree rotations isn't supported on any of them. */ u32 intel_fb_xy_to_linear(int x, int y, - const struct drm_framebuffer *fb, int plane) + const struct intel_plane_state *state, + int plane) { + const struct drm_framebuffer *fb = state->base.fb; unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, plane); unsigned int pitch = fb->pitches[plane]; @@ -2328,11 +2330,12 @@ u32 intel_fb_xy_to_linear(int x, int y, * specify the start of scanout from the beginning of the gtt mapping. */ void intel_add_fb_offsets(int *x, int *y, - const struct drm_framebuffer *fb, int plane, - unsigned int rotation) + const struct intel_plane_state *state, + int plane) { - const struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); + const struct intel_framebuffer *intel_fb = to_intel_framebuffer(state->base.fb); + unsigned int rotation = state->base.rotation; if (intel_rotation_90_or_270(rotation)) { *x += intel_fb->rotated[plane].x; @@ -2439,10 +2442,12 @@ static u32 _intel_compute_tile_offset(const struct drm_i915_private *dev_priv, } u32 intel_compute_tile_offset(int *x, int *y, - const struct drm_framebuffer *fb, int plane, - unsigned int rotation) + const struct intel_plane_state *state, + int plane) { - const struct drm_i915_private *dev_priv = to_i915(fb->dev); + const struct drm_i915_private *dev_priv = to_i915(state->base.plane->dev); + const struct drm_framebuffer *fb = state->base.fb; + unsigned int rotation = state->base.rotation; u32 alignment = intel_surf_alignment(dev_priv, fb->modifier[plane]); int pitch = intel_fb_pitch(fb, plane, rotation); @@ -2867,11 +2872,11 @@ static void i9xx_update_primary_plane(struct drm_plane *primary, if (IS_G4X(dev)) dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE; - intel_add_fb_offsets(, , fb, 0, rotation); + intel_add_fb_offsets(, , plane_state, 0); if (INTEL_INFO(dev)->gen >= 4) intel_crtc->dspaddr_offset = - intel_compute_tile_offset(, , fb, 0, rotation); + intel_compute_tile_offset(, , plane_state, 0); if (rotation == BIT(DRM_ROTATE_180)) { dspcntr |= DISPPLANE_ROTATE_180; @@ -2880,7 +2885,7 @@ static void i9xx_update_primary_plane(struct drm_plane *primary, y += (crtc_state->pipe_src_h - 1); } - linear_offset = intel_fb_xy_to_linear(x, y, fb, 0); + linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0); if (INTEL_INFO(dev)->gen < 4) intel_crtc->dspaddr_offset = linear_offset; @@ -2970,10 +2975,10 @@ static void ironlake_update_primary_plane(struct drm_plane *primary, if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE; - intel_add_fb_offsets(, , fb, 0, rotation); + intel_add_fb_offsets(, , plane_state, 0); intel_crtc->dspaddr_offset = - intel_compute_tile_offset(, , fb, 0, rotation); + intel_compute_tile_offset(, , plane_state, 0); if (rotation == BIT(DRM_ROTATE_180)) { dspcntr |= DISPPLANE_ROTATE_180; @@ -2984,7 +2989,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary, } } - linear_offset = intel_fb_xy_to_linear(x, y, fb, 0); + linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0); intel_crtc->adjusted_x = x;
Re: [Intel-gfx] [PATCH 03/12] drm/i915: Move SKL hw stride calculation into a helper
Reviewed-by: Sivakumar ThulasimaniOn 5/3/2016 9:09 PM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä We repeat the SKL stride register value calculations a several places. Move it into a small helper function. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 52 +--- drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_sprite.c | 12 ++--- 3 files changed, 29 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 16ac14a93776..87b783c56533 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3059,6 +3059,28 @@ static void skl_detach_scalers(struct intel_crtc *intel_crtc) } } +u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane, +unsigned int rotation) +{ + const struct drm_i915_private *dev_priv = to_i915(fb->dev); + u32 stride = intel_fb_pitch(fb, plane, rotation); + + /* +* The stride is either expressed as a multiple of 64 bytes chunks for +* linear buffers or in number of tiles for tiled buffers. +*/ + if (intel_rotation_90_or_270(rotation)) { + int cpp = drm_format_plane_cpp(fb->pixel_format, plane); + + stride /= intel_tile_height(dev_priv, fb->modifier[0], cpp); + } else { + stride /= intel_fb_stride_alignment(dev_priv, fb->modifier[0], + fb->pixel_format); + } + + return stride; +} + u32 skl_plane_ctl_format(uint32_t pixel_format) { switch (pixel_format) { @@ -3149,8 +3171,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane, struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); struct drm_framebuffer *fb = plane_state->base.fb; int pipe = intel_crtc->pipe; - u32 plane_ctl, stride; + u32 plane_ctl; unsigned int rotation = plane_state->base.rotation; + u32 stride = skl_plane_stride(fb, 0, rotation); u32 surf_addr; int scaler_id = plane_state->scaler_id; int src_x = plane_state->src.x1 >> 16; @@ -3178,8 +3201,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane, .y1 = src_y, .y2 = src_y + src_h, }; - int cpp = drm_format_plane_cpp(fb->pixel_format, 0); - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); /* Rotate src coordinates to match rotated GTT view */ drm_rect_rotate(, fb->width, fb->height, BIT(DRM_ROTATE_270)); @@ -3188,13 +3209,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane, src_y = r.y1; src_w = drm_rect_width(); src_h = drm_rect_height(); - - stride = intel_fb->rotated[0].pitch / - intel_tile_height(dev_priv, fb->modifier[0], cpp); - } else { - stride = fb->pitches[0] / - intel_fb_stride_alignment(dev_priv, fb->modifier[0], - fb->pixel_format); } intel_add_fb_offsets(_x, _y, fb, 0, rotation); @@ -11432,7 +11446,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, struct drm_i915_private *dev_priv = dev->dev_private; struct drm_framebuffer *fb = intel_crtc->base.primary->fb; const enum pipe pipe = intel_crtc->pipe; - u32 ctl, stride; + u32 ctl, stride = skl_plane_stride(fb, 0, rotation); ctl = I915_READ(PLANE_CTL(pipe, 0)); ctl &= ~PLANE_CTL_TILED_MASK; @@ -11453,22 +11467,6 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, } /* -* The stride is either expressed as a multiple of 64 bytes chunks for -* linear buffers or in number of tiles for tiled buffers. -*/ - if (intel_rotation_90_or_270(rotation)) { - int cpp = drm_format_plane_cpp(fb->pixel_format, 0); - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); - - stride = intel_fb->rotated[0].pitch / - intel_tile_height(dev_priv, fb->modifier[0], cpp); - } else { - stride = fb->pitches[0] / - intel_fb_stride_alignment(dev_priv, fb->modifier[0], - fb->pixel_format); - } - - /* * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on * PLANE_SURF updates, the update is then guaranteed to be atomic. */ diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 5045fea0c1c5..42b21c54a646 100644 ---
Re: [Intel-gfx] [PATCH v3 02/12] drm/i915: Don't pass pitch to intel_compute_page_offset()
Reviewed-by: Sivakumar ThulasimaniOn 5/3/2016 9:09 PM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä intel_compute_page_offset() can dig up the correct pitch from the fb itself, no need for the caller to pass it in. A bit of extra care is needed for the lower level _intel_compute_page_offset() since that one gets called before the rotated pitch under intel_fb is populated. Note that we don't actually call it with anything but DRM_ROTATE_0 there so we wouldn't actually look up the rotated pitch there, but still, leave the pitch as something the caller has to pass to _intel_compute_page_offset() as an indicator that something is a bit special. This leaves 'stride_div' in the skl plane update hooks as a mostly useless variable so just get rid of it. v2: Add a note why stride_div got nuked v3: Extract intel_fb_pitch() since it can be useful later Signed-off-by: Ville Syrjälä Reviewed-by: Daniel Vetter (v2) --- drivers/gpu/drm/i915/intel_display.c | 34 -- drivers/gpu/drm/i915/intel_drv.h | 1 - drivers/gpu/drm/i915/intel_sprite.c | 26 +++--- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3c95a3663269..16ac14a93776 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2298,6 +2298,15 @@ void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation) i915_gem_object_unpin_from_display_plane(obj, ); } +static int intel_fb_pitch(const struct drm_framebuffer *fb, int plane, + unsigned int rotation) +{ + if (intel_rotation_90_or_270(rotation)) + return to_intel_framebuffer(fb)->rotated[plane].pitch; + else + return fb->pitches[plane]; +} + /* * Convert the x/y offsets into a linear offset. * Only valid with 0/180 degree rotation, which is fine since linear @@ -2431,11 +2440,11 @@ static u32 _intel_compute_tile_offset(const struct drm_i915_private *dev_priv, u32 intel_compute_tile_offset(int *x, int *y, const struct drm_framebuffer *fb, int plane, - unsigned int pitch, unsigned int rotation) { const struct drm_i915_private *dev_priv = to_i915(fb->dev); u32 alignment = intel_surf_alignment(dev_priv, fb->modifier[plane]); + int pitch = intel_fb_pitch(fb, plane, rotation); return _intel_compute_tile_offset(dev_priv, x, y, fb, plane, pitch, rotation, alignment); @@ -2862,8 +2871,7 @@ static void i9xx_update_primary_plane(struct drm_plane *primary, if (INTEL_INFO(dev)->gen >= 4) intel_crtc->dspaddr_offset = - intel_compute_tile_offset(, , fb, 0, - fb->pitches[0], rotation); + intel_compute_tile_offset(, , fb, 0, rotation); if (rotation == BIT(DRM_ROTATE_180)) { dspcntr |= DISPPLANE_ROTATE_180; @@ -2965,8 +2973,7 @@ static void ironlake_update_primary_plane(struct drm_plane *primary, intel_add_fb_offsets(, , fb, 0, rotation); intel_crtc->dspaddr_offset = - intel_compute_tile_offset(, , fb, 0, - fb->pitches[0], rotation); + intel_compute_tile_offset(, , fb, 0, rotation); if (rotation == BIT(DRM_ROTATE_180)) { dspcntr |= DISPPLANE_ROTATE_180; @@ -3142,7 +3149,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane, struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); struct drm_framebuffer *fb = plane_state->base.fb; int pipe = intel_crtc->pipe; - u32 plane_ctl, stride_div, stride; + u32 plane_ctl, stride; unsigned int rotation = plane_state->base.rotation; u32 surf_addr; int scaler_id = plane_state->scaler_id; @@ -3182,17 +3189,16 @@ static void skylake_update_primary_plane(struct drm_plane *plane, src_w = drm_rect_width(); src_h = drm_rect_height(); - stride_div = intel_tile_height(dev_priv, fb->modifier[0], cpp); - stride = intel_fb->rotated[0].pitch; + stride = intel_fb->rotated[0].pitch / + intel_tile_height(dev_priv, fb->modifier[0], cpp); } else { - stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0], - fb->pixel_format); - stride = fb->pitches[0]; + stride = fb->pitches[0] / + intel_fb_stride_alignment(dev_priv, fb->modifier[0], +
Re: [Intel-gfx] [PATCH v5 01/12] drm/i915: Rewrite fb rotation GTT handling
On 5/3/2016 9:09 PM, ville.syrj...@linux.intel.com wrote: From: Ville SyrjäläRedo the fb rotation handling in order to: - eliminate the NV12 special casing - handle fb->offsets[] properly - make the rotation handling reasier for the plane code typo "easier" :) thanks for uploading again. Reviewed-by: Sivakumar Thulasimani regards, Sivakumar To achieve these goals we reduce intel_rotation_info to only contain (for each plane) the rotated view width,height,stride in tile units, and the page offset into the object where the plane starts. Each plane is handled exactly the same way, no special casing for NV12 or other formats. We then store the computed rotation_info under intel_framebuffer so that we don't have to recompute it again. To handle fb->offsets[] we treat them as a linear offsets and convert them to x/y offsets from the start of the relevant GTT mapping (either normal or rotated). We store the x/y offsets under intel_framebuffer, and for some extra convenience we also store the rotated pitch (ie. tile aligned plane height). So for each plane we have the normal x/y offsets, rotated x/y offsets, and the rotated pitch. The normal pitch is available already in fb->pitches[]. While we're gathering up all that extra information, we can also easily compute the storage requirements for the framebuffer, so that we can check that the object is big enough to hold it. When it comes time to deal with the plane source coordinates, we first rotate the clipped src coordinates to match the relevant GTT view orientation, then add to them the fb x/y offsets. Next we compute the aligned surface page offset, and as a result we're left with some residual x/y offsets. Finally, if required by the hardware, we convert the remaining x/y offsets into a linear offset. For gen2/3 we simply skip computing the final page offset, and just convert the src+fb x/y offsets directly into a linear offset since that's what the hardware wants. After this all platforms, incluing SKL+, compute these things in exactly the same way (excluding alignemnt differences). v2: Use BIT(DRM_ROTATE_270) instead of ROTATE_270 when rotating plane src coordinates Drop some spurious changes that got left behind during development v3: Split out more changes to prep patches (Daniel) s/intel_fb->plane[].foo.bar/intel_fb->foo[].bar/ for brevity Rename intel_surf_gtt_offset to intel_fb_gtt_offset Kill the pointless 'plane' parameter from intel_fb_gtt_offset() v4: Fix alignment vs. alignment-1 when calling _intel_compute_tile_offset() from intel_fill_fb_info() Pass the pitch in tiles in stad of pixels to intel_adjust_tile_offset() from intel_fill_fb_info() Pass the full width/height of the rotated area to drm_rect_rotate() for clarity Use u32 for more offsets v5: Preserve the upper_32_bits()/lower_32_bits() handling for the fb ggtt offset (Sivakumar) Cc: Sivakumar Thulasimani Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 5 +- drivers/gpu/drm/i915/intel_display.c | 368 --- drivers/gpu/drm/i915/intel_drv.h | 19 +- drivers/gpu/drm/i915/intel_sprite.c | 97 - 5 files changed, 331 insertions(+), 209 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 364cf8236021..7210cffcbaeb 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3381,16 +3381,14 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, } static struct sg_table * -intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, +intel_rotate_fb_obj_pages(const struct intel_rotation_info *rot_info, struct drm_i915_gem_object *obj) { - unsigned int size_pages = rot_info->plane[0].width * rot_info->plane[0].height; - unsigned int size_pages_uv; + unsigned int size = intel_rotation_info_size(rot_info); struct sg_page_iter sg_iter; unsigned long i; dma_addr_t *page_addr_list; struct sg_table *st; - unsigned int uv_start_page; struct scatterlist *sg; int ret = -ENOMEM; @@ -3401,18 +3399,12 @@ intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, if (!page_addr_list) return ERR_PTR(ret); - /* Account for UV plane with NV12. */ - if (rot_info->pixel_format == DRM_FORMAT_NV12) - size_pages_uv = rot_info->plane[1].width * rot_info->plane[1].height; - else - size_pages_uv = 0; - /* Allocate target SG list. */ st = kmalloc(sizeof(*st), GFP_KERNEL); if (!st) goto err_st_alloc; - ret = sg_alloc_table(st, size_pages + size_pages_uv, GFP_KERNEL); +
Re: [Intel-gfx] [PATCH v5 01/12] drm/i915: Rewrite fb rotation GTT handling
On 5/3/2016 9:09 PM, ville.syrj...@linux.intel.com wrote: From: Ville SyrjäläRedo the fb rotation handling in order to: - eliminate the NV12 special casing - handle fb->offsets[] properly - make the rotation handling reasier for the plane code typo "easier" :) otherwise fine. thanks for uploading the patches again. regards, Sivakumar To achieve these goals we reduce intel_rotation_info to only contain (for each plane) the rotated view width,height,stride in tile units, and the page offset into the object where the plane starts. Each plane is handled exactly the same way, no special casing for NV12 or other formats. We then store the computed rotation_info under intel_framebuffer so that we don't have to recompute it again. To handle fb->offsets[] we treat them as a linear offsets and convert them to x/y offsets from the start of the relevant GTT mapping (either normal or rotated). We store the x/y offsets under intel_framebuffer, and for some extra convenience we also store the rotated pitch (ie. tile aligned plane height). So for each plane we have the normal x/y offsets, rotated x/y offsets, and the rotated pitch. The normal pitch is available already in fb->pitches[]. While we're gathering up all that extra information, we can also easily compute the storage requirements for the framebuffer, so that we can check that the object is big enough to hold it. When it comes time to deal with the plane source coordinates, we first rotate the clipped src coordinates to match the relevant GTT view orientation, then add to them the fb x/y offsets. Next we compute the aligned surface page offset, and as a result we're left with some residual x/y offsets. Finally, if required by the hardware, we convert the remaining x/y offsets into a linear offset. For gen2/3 we simply skip computing the final page offset, and just convert the src+fb x/y offsets directly into a linear offset since that's what the hardware wants. After this all platforms, incluing SKL+, compute these things in exactly the same way (excluding alignemnt differences). v2: Use BIT(DRM_ROTATE_270) instead of ROTATE_270 when rotating plane src coordinates Drop some spurious changes that got left behind during development v3: Split out more changes to prep patches (Daniel) s/intel_fb->plane[].foo.bar/intel_fb->foo[].bar/ for brevity Rename intel_surf_gtt_offset to intel_fb_gtt_offset Kill the pointless 'plane' parameter from intel_fb_gtt_offset() v4: Fix alignment vs. alignment-1 when calling _intel_compute_tile_offset() from intel_fill_fb_info() Pass the pitch in tiles in stad of pixels to intel_adjust_tile_offset() from intel_fill_fb_info() Pass the full width/height of the rotated area to drm_rect_rotate() for clarity Use u32 for more offsets v5: Preserve the upper_32_bits()/lower_32_bits() handling for the fb ggtt offset (Sivakumar) Cc: Sivakumar Thulasimani Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 5 +- drivers/gpu/drm/i915/intel_display.c | 368 --- drivers/gpu/drm/i915/intel_drv.h | 19 +- drivers/gpu/drm/i915/intel_sprite.c | 97 - 5 files changed, 331 insertions(+), 209 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 364cf8236021..7210cffcbaeb 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3381,16 +3381,14 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, } static struct sg_table * -intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, +intel_rotate_fb_obj_pages(const struct intel_rotation_info *rot_info, struct drm_i915_gem_object *obj) { - unsigned int size_pages = rot_info->plane[0].width * rot_info->plane[0].height; - unsigned int size_pages_uv; + unsigned int size = intel_rotation_info_size(rot_info); struct sg_page_iter sg_iter; unsigned long i; dma_addr_t *page_addr_list; struct sg_table *st; - unsigned int uv_start_page; struct scatterlist *sg; int ret = -ENOMEM; @@ -3401,18 +3399,12 @@ intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, if (!page_addr_list) return ERR_PTR(ret); - /* Account for UV plane with NV12. */ - if (rot_info->pixel_format == DRM_FORMAT_NV12) - size_pages_uv = rot_info->plane[1].width * rot_info->plane[1].height; - else - size_pages_uv = 0; - /* Allocate target SG list. */ st = kmalloc(sizeof(*st), GFP_KERNEL); if (!st) goto err_st_alloc; - ret = sg_alloc_table(st, size_pages + size_pages_uv, GFP_KERNEL); + ret = sg_alloc_table(st, size,
Re: [Intel-gfx] FW: [PATCH v4 10/21] drm/i915: Rewrite fb rotation GTT handling
sorry for the huge delay in reviewing this series, have just started on this and could review only the first patch for today, hopefully i'll continue on this everyday this week :). replying as a detached thread since i lost the mails as part original series so please forgive for the detached reply for review. On 5/2/2016 3:20 PM, Mukherjee, Indranil wrote: Regards, Indranil -Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of ville.syrj...@linux.intel.com Sent: Tuesday, February 16, 2016 2:25 AM To: intel-gfx@lists.freedesktop.org Subject: [Intel-gfx] [PATCH v4 10/21] drm/i915: Rewrite fb rotation GTT handling From: Ville SyrjäläRedo the fb rotation handling in order to: - eliminate the NV12 special casing - handle fb->offsets[] properly - make the rotation handling reasier for the plane code typo "easier" To achieve these goals we reduce intel_rotation_info to only contain (for each plane) the rotated view width,height,stride in tile units, and the page offset into the object where the plane starts. Each plane is handled exactly the same way, no special casing for NV12 or other formats. We then store the computed rotation_info under intel_framebuffer so that we don't have to recompute it again. To handle fb->offsets[] we treat them as a linear offsets and convert them to x/y offsets from the start of the relevant GTT mapping (either normal or rotated). We store the x/y offsets under intel_framebuffer, and for some extra convenience we also store the rotated pitch (ie. tile aligned plane height). So for each plane we have the normal x/y offsets, rotated x/y offsets, and the rotated pitch. The normal pitch is available already in fb->pitches[]. While we're gathering up all that extra information, we can also easily compute the storage requirements for the framebuffer, so that we can check that the object is big enough to hold it. When it comes time to deal with the plane source coordinates, we first rotate the clipped src coordinates to match the relevant GTT view orientation, then add to them the fb x/y offsets. Next we compute the aligned surface page offset, and as a result we're left with some residual x/y offsets. Finally, if required by the hardware, we convert the remaining x/y offsets into a linear offset. For gen2/3 we simply skip computing the final page offset, and just convert the src+fb x/y offsets directly into a linear offset since that's what the hardware wants. After this all platforms, incluing SKL+, compute these things in exactly the same way (excluding alignemnt differences). v2: Use BIT(DRM_ROTATE_270) instead of ROTATE_270 when rotating plane src coordinates Drop some spurious changes that got left behind during development v3: Split out more changes to prep patches (Daniel) s/intel_fb->plane[].foo.bar/intel_fb->foo[].bar/ for brevity Rename intel_surf_gtt_offset to intel_fb_gtt_offset Kill the pointless 'plane' parameter from intel_fb_gtt_offset() v4: Fix alignment vs. alignment-1 when calling _intel_compute_tile_offset() from intel_fill_fb_info() Pass the pitch in tiles in stad of pixels to intel_adjust_tile_offset() from intel_fill_fb_info() Pass the full width/height of the rotated area to drm_rect_rotate() for clarity Use u32 for more offsets Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 5 +- drivers/gpu/drm/i915/intel_display.c | 373 +++ drivers/gpu/drm/i915/intel_drv.h | 19 +- drivers/gpu/drm/i915/intel_sprite.c | 97 - 5 files changed, 331 insertions(+), 214 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 51f2597e3c56..5b17944a512b 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3395,16 +3395,14 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, } static struct sg_table * -intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, +intel_rotate_fb_obj_pages(const struct intel_rotation_info *rot_info, struct drm_i915_gem_object *obj) { - unsigned int size_pages = rot_info->plane[0].width * rot_info->plane[0].height; - unsigned int size_pages_uv; + unsigned int size = intel_rotation_info_size(rot_info); struct sg_page_iter sg_iter; unsigned long i; dma_addr_t *page_addr_list; struct sg_table *st; - unsigned int uv_start_page; struct scatterlist *sg; int ret = -ENOMEM; @@ -3414,18 +3412,12 @@ intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, if (!page_addr_list) return ERR_PTR(ret); - /* Account for UV plane with NV12. */ - if (rot_info->pixel_format ==
Re: [Intel-gfx] [PATCH 2/5] drm/i915: Read test values for lane_count and link_rate
On 5/2/2016 2:38 PM, Shubhangi Shrivastava wrote: This patch is intended to read test values only. Call to intel_dp_start_link_train will be in upcoming patch "Lane count change detection", which I will be posting after this patch series.. On Tuesday 26 April 2016 09:39 AM, Navare, Manasi D wrote: The automated test request for link training needs to start the link training with the requested link rate and lane count. So after reading the TEST LANE COUNT and TEST LINK RATE values, it needs to call intel_dp_start_link_train() also. How is the automated link train being tested currently? Could you add some details of the automated testing (test numbers from the CTS usite) in the commit message. Regards, Manasi Navare Graphics Kernel Developer OTC, Intel Corporation Almost all link training tests in CTS use the TEST LANE COUNT and TEST LINK RATE values. i.e 4.3.1.1 to 4.3.1.10 and 4.3.2.1 to 4.3.2.6. This was tested using DPR100 & DPR120 compliance test tool from Unigraf. As mentioned by Shubhangi this is just a clean up patch and further patches will complete the process of using the test values in next link training. To give some background, if you run either of the tools mentioned above, they give us a short/long pulse with TEST_LINK_TRAIN request bit set, we are supposed to then read the link rate and lane count values from appropriate Test DPCD registers and use them in the following link training. So as per compliance requirements you need to use these values in the next link training (which is assumed to be as part of modeset if we got it as part of long pulse and for short pulse we can explicitly retrain the link) regards, Sivakumar -Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Shubhangi Shrivastava Sent: Monday, April 25, 2016 1:24 AM To: intel-gfx@lists.freedesktop.org Cc: Shrivastava, Shubhangi Subject: [Intel-gfx] [PATCH 2/5] drm/i915: Read test values for lane_count and link_rate During automated test request for link training we are supposed to read the TEST_LANE_COUNT and TEST_LINK_RATE dpcd registers and use respective values in the next link training. This patch adds reading and updating of these values. Signed-off-by: Sivakumar ThulasimaniSigned-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/intel_dp.c | 25 + 1 file changed, 25 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 1b26c59..387800b 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4010,9 +4010,34 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) return true; } +/* + * This function reads TEST_LANE_COUNT & TEST_LINK_RATE and updates + * them to cached dpcd values, thus the new values are implicitly + * used by rest of the code without need to be aware of the change. + */ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp) { uint8_t test_result = DP_TEST_ACK; +uint8_t dpcd_val, ret; + +ret = drm_dp_dpcd_read(_dp->aux, + DP_TEST_LANE_COUNT, + _val, 1); + +/* update values only if read returned 1 byte */ +if (ret == 1) { +dpcd_val &= DP_MAX_LANE_COUNT_MASK; +intel_dp->dpcd[DP_MAX_LANE_COUNT] &= ~(DP_MAX_LANE_COUNT_MASK); +intel_dp->dpcd[DP_MAX_LANE_COUNT] |= dpcd_val; +} + +ret = drm_dp_dpcd_read(_dp->aux, + DP_TEST_LINK_RATE, + _val, 1); + +if (ret == 1) +intel_dp->dpcd[DP_MAX_LINK_RATE] = dpcd_val; + return test_result; } -- 2.6.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915: Splitting intel_dp_detect
On 4/7/2016 1:54 PM, Ander Conselvan De Oliveira wrote: On Thu, 2016-04-07 at 10:58 +0300, Jani Nikula wrote: On Wed, 06 Apr 2016, Tvrtko Ursulinwrote: On 04/04/16 12:41, Tvrtko Ursulin wrote: On 04/04/16 12:08, Jani Nikula wrote: On Mon, 04 Apr 2016, Tvrtko Ursulin wrote: On 01/04/16 08:41, Ander Conselvan De Oliveira wrote: On Thu, 2016-03-31 at 12:38 +, Patchwork wrote: == Series Details == Series: series starting with [1/5] drm/i915: Splitting intel_dp_detect URL : https://patchwork.freedesktop.org/series/5044/ State : success I pushed those to dinq. This series seems to break eDP detection on BDW RVP. I presume this is due to the sink count check. Can you add debug logging to print intel_dp->sink_count after it's been read in intel_dp_get_dpcd() please? intel_dp->sink_count is zero here. (raw value, before the DP_GET_SINK_COUNT.) Also, intel_dp_dpcd_read_wake suggests a possibility for reading garbage with not overly confident wording for the workaround there. Then the question is, is this just because you have an RVP with who knows what panel, or do we have to take into account potentially broken panels too? Then I assume the fix would be to to ignore sink count for eDP. No idea. :) I could really use a solution for this. My only development platform is incapacitated unless I revert this series which, apart from the extra work when preparing and sending out patches this is taking, including lost time waiting on CI which I suspect dislikes patches from top of unknown bases, I think it won't be so easy to continue doing so when the conflicts start arriving. :( Ander, Shubhangi? Would something like this be sensible? Tvrtko, can you give it a go? diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index da0c3d29fda8..0890e71db188 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3799,6 +3799,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) */ intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count); + if (is_edp(intel_dp)) + intel_dp->sink_count = max(intel_dp->sink_count, 1); I couldn't find anything in the spec that would make SINK_COUNT behave differently for eDP, but eDP with 0 sinks simply doesn't make sense, so this seems sensible to me. Ander its possible that manufacturers might not have filled sink count dpcd registers. i would prefer ignoring sink_count for edp rather than hardcoding 1 in case of edp. Also just to be safe, we should add a similar check in short pulse handling too where we check sink count. Shubhangi, can you share a patch to handle this asap ? regards, Sivakumar /* * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that * a dongle is present but no display. Unless we require to know BR, Jani. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] Enable FBC on SKL, v3
On 4/6/2016 7:24 PM, Zanoni, Paulo R wrote: Em Qua, 2016-04-06 às 10:36 +0530, Thulasimani, Sivakumar escreveu: dont want to hijack thread but wanted to point out a possible regression in the previous patches of this series. intel_fbc_can_choose: returns true for gen 4/5/6/7. (possible bug) How? It will check for i915.enable_fbc, which will have been sanitized to zero on these platforms. Aren't you explicitly enabling FBC on these platforms by using i915.enable_fbc=1? nope, found as part of code walkthrough hence mentioned above that it is "possible regression" :) . Two points > found that this is not a regression per say as it was there before your change too > Agree that FBC code wont reach intel_fbc_can_choose due to platform checks but without proper comments wont someone assume that the rest of code should work properly especially when we have checks for non haswell/bdw platforms in the code flow. Also found another possible bug in multiple_pipes_ok. i assume it is supposed to return true when we can enable FBC after considering multiple pipes are enabled or not. but it returns false in case of single display in gen3 thus ending up doing opposite of multiple pipe requirement. i understand all these are on very old platforms which dont have FBC enabled at all so fixing them should be lower priority. but sharing them here so that at-least it is documented somewhere. regards, Sivakumar so intel_crtc_state->enable_fbc = true; will be executed for first crtc everytime intel_fbc_choose_crtc is called. Although there is check to handle fbc already enabled, it may fail when we fbc is disabled and we are working on non supported panel. regards, Sivakumar On 4/5/2016 2:47 AM, Paulo Zanoni wrote: Now with the suggestion from Chris instead of the old workaround. We don't need new DDX patches anymore, but now we need new IGT patches. Chris Wilson (1): drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps Paulo Zanoni (3): drm/i915/fbc: update busy_bits even for GTT and flip flushes drm/i915/fbc: sanitize i915.enable_fbc during FBC init drm/i915/fbc: enable FBC on gen 9+ too drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 14 +++--- drivers/gpu/drm/i915/intel_fbc.c | 27 --- 3 files changed, 28 insertions(+), 14 deletions(-) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] Enable FBC on SKL, v3
dont want to hijack thread but wanted to point out a possible regression in the previous patches of this series. intel_fbc_can_choose: returns true for gen 4/5/6/7. (possible bug) so intel_crtc_state->enable_fbc = true; will be executed for first crtc everytime intel_fbc_choose_crtc is called. Although there is check to handle fbc already enabled, it may fail when we fbc is disabled and we are working on non supported panel. regards, Sivakumar On 4/5/2016 2:47 AM, Paulo Zanoni wrote: Now with the suggestion from Chris instead of the old workaround. We don't need new DDX patches anymore, but now we need new IGT patches. Chris Wilson (1): drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps Paulo Zanoni (3): drm/i915/fbc: update busy_bits even for GTT and flip flushes drm/i915/fbc: sanitize i915.enable_fbc during FBC init drm/i915/fbc: enable FBC on gen 9+ too drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 14 +++--- drivers/gpu/drm/i915/intel_fbc.c | 27 --- 3 files changed, 28 insertions(+), 14 deletions(-) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting
On 3/1/2016 5:03 AM, Rob Clark wrote: On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <dan...@ffwll.ch> wrote: On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote: On 2/24/2016 3:41 AM, Lyude wrote: As it turns out, resuming DP MST is racey since we don't make sure MST is ready before we start modesetting, it just usually happens to be ready in time. This isn't the case on all systems, particularly a ThinkPad T560 with displays connected through the dock. On these systems, resuming the laptop while connected to the dock usually results in blank monitors. Making sure MST is ready before doing any kind of modesetting fixes this issue. basic question since i haven't worked on MST much. MST should work like any other digital panel on resume. i.e detect followed by modeset. in the modified commit mentioned below is it failing to detect the panel or failing at the modeset ? if we are depending on the intel_display_resume, how about moving the intel_dp_mst_resume just above intel_display_resume? Generic question to others in mail list on i915_drm_resume we are doing a modeset and then doing the detect/hpd init. shouldn't this be the other way round ? almost all displays will pass a modeset even if display is not connected so we are spending time on modeset even for displays that were removed during the suspend state. if this is to simply drm_state being saved and restored, We must restore anyway, we're not really allowed to shut down a display without userspace's consent. DP mst breaks this, and it's not fun really. well, that isn't completely true.. I mean, if the user unplugs (for example) an hdmi monitor, it isn't with userspace's consent.. I wonder if we could just have flag per connector, ie. connector->no_auto_resume and just automatically send unplug events for those to userspace (followed shortly by a plug event if it is still connected and the normal hpd mechanism kicks in? I mean, for all we know, the user unplugged the DP monitor/hub/etc and plugged in a different one while we were suspended.. BR, -R i agree. performing a modeset on a display without even confirming if it is the same one when we had suspended the system will result in issues such as applying a mode that may not be supported on the new display or corruption or blankout or simply failing the modeset restore or worst case of doing modeset without a display connected. if we will not allow such a scenario during normal operation, i.e prevent incorrect modeset requests during normal functioning, why allow such a modeset during suspend-resume alone ? we can store the edid hash and if the display has the same hash post resume then we can allow mode to be restored. regards, Sivakumar So for hotunplug the flow should always be: 1. kernel notices something has changed in the output config. 2. kernel sends out uevent 3. userspace figures out what changed, and what needs to be done 4. userspace asks the kernel to change display configuration through setCrtc and Atomic ioctl calls. Irrespective of hotunplug handling, the kernel also _must_ restore the entire display configuration before userspace resumes. We can do that asynchronously (and there's plans for that), but even then we must stall userspace on the first KMS ioclt to keep up the illusion that a system s/r is transparent. In short, even if we do hpd processing before resuming the display, nothing will be faster - we must wait for userspace to make up its mind, and that can only happen once we've restored the display config. And again, mst is kinda breaking this, since and mst unplug results in a force-disable. Which can upset userspace and in general results in the need for lots of fragile error handling all over. We originally changed the resume order in commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state") to fix a ton of WARN_ON's after resume, but this doesn't seem to be an issue now anyhow. CC: sta...@vger.kernel.org Signed-off-by: Lyude <cp...@redhat.com> Wrt the patch itself: I think only in 4.6 we've actually fixed up all these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the modeset state readout for mst is still not entirely correct (it still relies on software state), so I'm not sure we've actually managed to shut up all the WARNINGs. Iirc the way to repro them is to hot-unplug something while suspended. In short the get_hw_state functions for mst should not rely on any mst software state, but instead just look at hw registers and dp aux registers to figure out what's going on. But not sure whether this will result on WARNINGs on resume, since generally the bios doesn't enable anything in that case. Furthermore MST still does a force-modeset when processing a hotunplug. Doing that before we've resumed the display is likely a very bad idea. What we need to fix that part is to separate the dp mst connector hotplug/unplugging from actually updating the modeset cha
Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting
On 2/24/2016 3:41 AM, Lyude wrote: As it turns out, resuming DP MST is racey since we don't make sure MST is ready before we start modesetting, it just usually happens to be ready in time. This isn't the case on all systems, particularly a ThinkPad T560 with displays connected through the dock. On these systems, resuming the laptop while connected to the dock usually results in blank monitors. Making sure MST is ready before doing any kind of modesetting fixes this issue. basic question since i haven't worked on MST much. MST should work like any other digital panel on resume. i.e detect followed by modeset. in the modified commit mentioned below is it failing to detect the panel or failing at the modeset ? if we are depending on the intel_display_resume, how about moving the intel_dp_mst_resume just above intel_display_resume? Generic question to others in mail list on i915_drm_resume we are doing a modeset and then doing the detect/hpd init. shouldn't this be the other way round ? almost all displays will pass a modeset even if display is not connected so we are spending time on modeset even for displays that were removed during the suspend state. if this is to simply drm_state being saved and restored, We originally changed the resume order in commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state") to fix a ton of WARN_ON's after resume, but this doesn't seem to be an issue now anyhow. CC: sta...@vger.kernel.org Signed-off-by: Lyude--- drivers/gpu/drm/i915/i915_drv.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index f357058..4dcf3dd 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev) intel_opregion_setup(dev); intel_init_pch_refclk(dev); + + /* +* We need to make sure that we resume MST before doing anything +* display related, otherwise we risk trying to bring up a display on +* MST before the hub is actually ready +*/ + intel_dp_mst_resume(dev); + This does not look proper. if the CD clock is turned off during suspend our dpcd read itself might fail till we enable CD Clock. regards, Sivakumar drm_mode_config_reset(dev); /* @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev) intel_display_resume(dev); drm_modeset_unlock_all(dev); - intel_dp_mst_resume(dev); - /* * ... but also need to make sure that hotplug processing * doesn't cause havoc. Like in the driver load code we don't ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW
On 2/23/2016 4:35 PM, Ville Syrjälä wrote: On Tue, Feb 23, 2016 at 04:22:01PM +0530, Thulasimani, Sivakumar wrote: On 2/16/2016 3:35 PM, Ville Syrjälä wrote: On Tue, Feb 16, 2016 at 07:13:05AM +0530, Thulasimani, Sivakumar wrote: On 2/12/2016 8:36 PM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä <ville.syrj...@linux.intel.com> Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or up to 675 MHz on ULT, bu only if extra cooling is provided. There don't seem to be any strap or VBT bits to tells us this however. But I did spot something potentially relevant in VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware knows what its doing and trust the max cdclk in SWF06 if it's higher than the basic limit specified in Bspec. To avoid regressing anything let's ignore SWF06 if it indicates a lower limit than Bspec. Cc: Clint Taylor <clinton.a.tay...@intel.com> Cc: "Thulasimani, Sivakumar" <sivakumar.thulasim...@intel.com> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com> --- I'm not at all sure if this is the right way to go about it. Sivakumar, since you seem to have some ideas on this maybe you can have a look. I'm not aware of any complaints so far that people can't get the cdclk as high is they should on specific machines, so not sure if this is really even needed. The other open question is what we should do if the VBIOS limit is lower than what we'd expect based on BSpec. Should we still trust it? Sadly we can't verify the SWF06 cdclk value in any way since it only has two bits and we have four possible cdclk values. The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so it just backs up the CD Clock before it optimizes for the available LFP. if we are trusting for higher value we should trust it for lower value too. The OEM might have did this to either reduce max resolution for cheaper products or might have removed some cooling mechanisms for thinner designs since we cannot say which of the reasons we should trust the lower value too. now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver from intel alone(it is not programmed from VBT), since GOP driver can be implemented by anyone and if anyone implements their own GOP driver we cannot be sure if the value is valid or not. please check if we can check for "Intel GOP driver". And if "Intel GOP driver" was used during boot, we can trust the value 100%. i am not sure how this can be done, so i would recommend trusting the values with clear debug messages as done below already. We definitely need a way to validate the register value before we trust it for lower values. I suppose we might be able to look at bits 31:16 since those should store the current cdclk "decimal" value. If that part looks reasonable, we might be able to trust the "max cdclk" bits as well. it seems the bits 31:16 are used only by VBIOS and not GOP, so that wont help. we need to come up with some other method to confirm the value or verify if intel gop is loaded. i will get back if i can find such a mechanism. Oh, that's unfortunate. IIRC we use some other SWF register to check if something already initialized things in the cdclk sanitation path. Not sure if the same could be used here. i am not aware of any other SWF register used for cdclk related flow so cant help there. had a discussion with local folks and it seems like there is no easy way out atleast for BDW. SKL register using literal values is helpful in verifying against available set. my best guess would be to handle it for BDW is as follows if BIT0:1 == 0 && current_cdclk != 450MHz we are not in intel GOP driver so take the current clock as max if BIT0:1 == 0 && current_cdclk == 450MHz cant say which GOP driver was used, so limit to current clock as max if BIT0:1 != 0 probability of some other component setting these two bits is very low :) so assume that we are in intel gop driver and trust the value. we have checks to limit clock for the SKU so it should not be a problem to trust the value in register. regards, Sivakumar As mentioned in another thread, this needs to be done for SKL too. i dont have a SKL system so if no one else can make a patch for it i will have to share an untested patch for it :). For SKL it seems a bit easier, since it apparently just stores the max cdclk in 10.1 decimal fixed point in bits 10:0. So invalid/uninitialized values would be easier to spot. Hmm. 11 bits and 10.1 fixed point format, I wonder if it's really decimal and not binary fixed point... it uses the same value programmed in CDCLK_CTL register. i.e 01 0011 0011 1b= 308.57MHz OK, so binary fixed point. Maybe I should file a bug against bspec to remove this confusing "decimal" namin
Re: [Intel-gfx] [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW
On 2/16/2016 3:35 PM, Ville Syrjälä wrote: On Tue, Feb 16, 2016 at 07:13:05AM +0530, Thulasimani, Sivakumar wrote: On 2/12/2016 8:36 PM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä <ville.syrj...@linux.intel.com> Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or up to 675 MHz on ULT, bu only if extra cooling is provided. There don't seem to be any strap or VBT bits to tells us this however. But I did spot something potentially relevant in VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware knows what its doing and trust the max cdclk in SWF06 if it's higher than the basic limit specified in Bspec. To avoid regressing anything let's ignore SWF06 if it indicates a lower limit than Bspec. Cc: Clint Taylor <clinton.a.tay...@intel.com> Cc: "Thulasimani, Sivakumar" <sivakumar.thulasim...@intel.com> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com> --- I'm not at all sure if this is the right way to go about it. Sivakumar, since you seem to have some ideas on this maybe you can have a look. I'm not aware of any complaints so far that people can't get the cdclk as high is they should on specific machines, so not sure if this is really even needed. The other open question is what we should do if the VBIOS limit is lower than what we'd expect based on BSpec. Should we still trust it? Sadly we can't verify the SWF06 cdclk value in any way since it only has two bits and we have four possible cdclk values. The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so it just backs up the CD Clock before it optimizes for the available LFP. if we are trusting for higher value we should trust it for lower value too. The OEM might have did this to either reduce max resolution for cheaper products or might have removed some cooling mechanisms for thinner designs since we cannot say which of the reasons we should trust the lower value too. now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver from intel alone(it is not programmed from VBT), since GOP driver can be implemented by anyone and if anyone implements their own GOP driver we cannot be sure if the value is valid or not. please check if we can check for "Intel GOP driver". And if "Intel GOP driver" was used during boot, we can trust the value 100%. i am not sure how this can be done, so i would recommend trusting the values with clear debug messages as done below already. We definitely need a way to validate the register value before we trust it for lower values. I suppose we might be able to look at bits 31:16 since those should store the current cdclk "decimal" value. If that part looks reasonable, we might be able to trust the "max cdclk" bits as well. it seems the bits 31:16 are used only by VBIOS and not GOP, so that wont help. we need to come up with some other method to confirm the value or verify if intel gop is loaded. i will get back if i can find such a mechanism. As mentioned in another thread, this needs to be done for SKL too. i dont have a SKL system so if no one else can make a patch for it i will have to share an untested patch for it :). For SKL it seems a bit easier, since it apparently just stores the max cdclk in 10.1 decimal fixed point in bits 10:0. So invalid/uninitialized values would be easier to spot. Hmm. 11 bits and 10.1 fixed point format, I wonder if it's really decimal and not binary fixed point... it uses the same value programmed in CDCLK_CTL register. i.e 01 0011 0011 1b= 308.57MHz regards, Sivakumar drivers/gpu/drm/i915/intel_display.c | 60 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 836bbdc239b6..1d70f6bf2934 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev) dev_priv->max_cdclk_freq = 45; else dev_priv->max_cdclk_freq = 337500; - } else if (IS_BROADWELL(dev)) { + } else if (IS_BROADWELL(dev)) { + int bios_max_cdclk_freq, max_cdclk_freq; + /* -* FIXME with extra cooling we can allow -* 540 MHz for ULX and 675 Mhz for ULT. -* How can we know if extra cooling is -* available? PCI ID, VTB, something else? +* With extra cooling we can allow 540 MHz for +* ULX and 675 Mhz for ULT. Assume VBIOS/GOP +* passes that information in SWF06. */ - if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) - dev_priv->max_cdclk_freq = 45; -
Re: [Intel-gfx] [PATCH V5] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
On 2/15/2016 6:46 PM, Ville Syrjälä wrote: On Fri, Feb 12, 2016 at 06:06:10PM -0800, clinton.a.tay...@intel.com wrote: From: Clint TaylorSet cdclk based on the max required pixel clock based on VCO selected. Track boot vco instead of boot cdclk. The vco is now tracked at the atomic level and all CRTCs updated if the required vco is changed. Not tested with eDP v1.4 panels that require 8640 vco due to availability. V1: initial version V2: add vco tracking in intel_dp_compute_config(), rename skl_boot_cdclk. V3: rebase, V2 feedback not possible as encoders are not aware of atomic. V4: track target vco is atomic state. modeset all CRTCs if vco changes V5: rename atomic variable, cleaner if/else logic, use existing vco if encoder does not return a new vco value. check_patch.pl cleanup Signed-off-by: Clint Taylor Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= --- drivers/gpu/drm/i915/i915_drv.h |2 +- drivers/gpu/drm/i915/intel_ddi.c |2 +- drivers/gpu/drm/i915/intel_display.c | 114 +- drivers/gpu/drm/i915/intel_dp.c |6 +- drivers/gpu/drm/i915/intel_drv.h |4 ++ 5 files changed, 111 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8216665..f65dd1a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1822,7 +1822,7 @@ struct drm_i915_private { int num_fence_regs; /* 8 on pre-965, 16 otherwise */ unsigned int fsb_freq, mem_freq, is_ddr3; - unsigned int skl_boot_cdclk; + unsigned int skl_vco_freq; unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq; unsigned int max_dotclk_freq; unsigned int hpll_freq; diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 6d5b09f..285adab 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev) int cdclk_freq; cdclk_freq = dev_priv->display.get_display_clock_speed(dev); - dev_priv->skl_boot_cdclk = cdclk_freq; + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq); if (skl_sanitize_cdclk(dev_priv)) DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9e2273b..c283abd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq) return (freq - 1000) / 500; } -static unsigned int skl_cdclk_get_vco(unsigned int freq) +unsigned int skl_cdclk_get_vco(unsigned int freq) { unsigned int i; @@ -5821,17 +5821,21 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) void skl_init_cdclk(struct drm_i915_private *dev_priv) { - unsigned int required_vco; + unsigned int cdclk; /* DPLL0 not enabled (happens on early BIOS versions) */ if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) { /* enable DPLL0 */ - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk); - skl_dpll0_enable(dev_priv, required_vco); + if (dev_priv->skl_vco_freq != 8640) + dev_priv->skl_vco_freq = 8100; + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq); + cdclk = ((dev_priv->skl_vco_freq == 8100) ? 337500 : 308570); + } else { + cdclk = dev_priv->cdclk_freq; } - /* set CDCLK to the frequency the BIOS chose */ - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk); + /* set CDCLK to the lowest frequency, Modeset follows */ + skl_set_cdclk(dev_priv, cdclk); /* enable DBUF power */ I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST); @@ -5847,7 +5851,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) { uint32_t lcpll1 = I915_READ(LCPLL1_CTL); uint32_t cdctl = I915_READ(CDCLK_CTL); - int freq = dev_priv->skl_boot_cdclk; + int freq = dev_priv->cdclk_freq; /* * check if the pre-os intialized the display @@ -5871,11 +5875,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) /* All well; nothing to sanitize */ return false; sanitize: - /* -* As of now initialize with max cdclk till -* we get dynamic cdclk support -* */ - dev_priv->skl_boot_cdclk = dev_priv->max_cdclk_freq; + skl_init_cdclk(dev_priv); /* we did have to sanitize */ @@ -9845,6 +9845,70 @@ static void broadwell_modeset_commit_cdclk(struct
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Set invert bit for hpd based on VBT
On 2/12/2016 7:38 PM, Ville Syrjälä wrote: On Fri, Feb 12, 2016 at 06:39:44PM +0530, Shubhangi Shrivastava wrote: This patch sets the invert bit for hpd detection for each port based on VBT configuration. Since each AOB can be designed to depend on invert bit or not, it is expected if an AOB requires invert bit, the user will set respective bit in VBT. v2: Separated VBT parsing from the rest of the logic. (Jani) Signed-off-by: Sivakumar ThulasimaniSigned-off-by: Durgadoss R Signed-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 43 +++ drivers/gpu/drm/i915/i915_reg.h | 9 drivers/gpu/drm/i915/intel_bios.c | 42 ++ 4 files changed, 95 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8216665..457f175 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3393,6 +3393,7 @@ extern void intel_i2c_reset(struct drm_device *dev); /* intel_bios.c */ int intel_bios_init(struct drm_i915_private *dev_priv); bool intel_bios_is_valid_vbt(const void *buf, size_t size); +bool intel_bios_is_port_hpd_inverted(struct drm_device *dev, enum port port); /* intel_opregion.c */ #ifdef CONFIG_ACPI diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 25a8937..fb95fb0 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -36,6 +36,7 @@ #include "i915_drv.h" #include "i915_trace.h" #include "intel_drv.h" +#include "intel_bios.h" /** * DOC: interrupt handling @@ -3424,6 +3425,47 @@ static void ibx_hpd_irq_setup(struct drm_device *dev) I915_WRITE(PCH_PORT_HOTPLUG, hotplug); } +/* + * For BXT invert bit has to be set based on AOB design + * for HPD detection logic, update it based on VBT fields. + */ +static void bxt_hpd_set_invert(struct drm_device *dev, u32 hotplug_port) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int reg_val, val = 0; + enum port port; + + for (port = PORT_A; port <= PORT_C; port++) { + + /* Proceed only if invert bit is set */ + if (intel_bios_is_port_hpd_inverted(dev, port)) { + switch (port) { + case PORT_A: + if (hotplug_port & BXT_DE_PORT_HP_DDIA) + val |= BXT_DDIA_HPD_INVERT; + break; + case PORT_B: + if (hotplug_port & BXT_DE_PORT_HP_DDIB) + val |= BXT_DDIB_HPD_INVERT; + break; + case PORT_C: + if (hotplug_port & BXT_DE_PORT_HP_DDIC) + val |= BXT_DDIC_HPD_INVERT; + break; + default: + DRM_ERROR("HPD invert set for invalid port %d\n", + port); + break; + } + } + } + reg_val = I915_READ(BXT_HOTPLUG_CTL); + DRM_DEBUG_KMS("Invert bit setting: hp_ctl:%x hp_port:%x val:%x\n", + reg_val, hotplug_port, val); + reg_val &= ~BXT_DDI_HPD_INVERT_MASK; + I915_WRITE(BXT_HOTPLUG_CTL, reg_val | val); +} No RMW stuff please. Just set up the bits appropriately in bxt_hpd_irq_setup(). the problem is we need to query vbt for setting invert bit. so not sure having this logic inside bxt_hpd_irq_setup is good. if we want to avoid read/write to registers we can modify the input to be values written on register instead of enabled_irqs. that way we can write the value to register post call to this function and we need not worry about current values in register. is that fine ? + static void spt_hpd_irq_setup(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3494,6 +3536,7 @@ static void bxt_hpd_irq_setup(struct drm_device *dev) hotplug |= PORTC_HOTPLUG_ENABLE | PORTB_HOTPLUG_ENABLE | PORTA_HOTPLUG_ENABLE; I915_WRITE(PCH_PORT_HOTPLUG, hotplug); + bxt_hpd_set_invert(dev, enabled_irqs); } static void ibx_irq_postinstall(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 6732fc1..66cf92e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5940,6 +5940,15 @@ enum skl_disp_power_wells { #define GEN8_PCU_IIR _MMIO(0x444e8) #define GEN8_PCU_IER _MMIO(0x444ec) +/* BXT hotplug control */ +#define BXT_HOTPLUG_CTL_MMIO(0xC4030)
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add Backlight Control using DPCD for eDP connectors (v6)
On 2/9/2016 6:41 PM, Adebisi, YetundeX wrote: -Original Message- From: Thulasimani, Sivakumar Sent: Monday, February 08, 2016 8:57 AM To: Adebisi, YetundeX; intel-gfx@lists.freedesktop.org Cc: Nikula, Jani Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add Backlight Control using DPCD for eDP connectors (v6) On 2/5/2016 5:48 PM, Yetunde Adebisi wrote: This patch adds support for eDP backlight control using DPCD registers to backlight hooks in intel_panel. It checks for backlight control over AUX channel capability and sets up function pointers to get and set the backlight brightness level if supported. v2: Moved backlight functions from intel_dp.c into a new file intel_dp_aux_backlight.c. Also moved reading of eDP display control registers to intel_dp_get_dpcd v3: Correct some formatting mistakes v4: Updated to use AUX backlight control if PWM control is not possible (Jani) v5: Moved call to initialize backlight registers to dp_aux_setup_backlight v6: Check DP_EDP_BACKLIGHT_PIN_ENABLE_CAP is disabled before setting up AUX backlight control. To fix BLM_PWM_ENABLE igt test warnings on bdw_ultra This patch depends on http://patchwork.freedesktop.org/patch/64253/ Cc: Bob Paauwe <bob.j.paa...@intel.com> Cc: Jani Nikula <jani.nik...@intel.com> Acked-by: Jesse Barnes <jbar...@virtuousgeek.org> Signed-off-by: Yetunde Adebisi <yetundex.adeb...@intel.com> --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/intel_dp.c | 17 ++- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 170 ++ drivers/gpu/drm/i915/intel_drv.h | 6 + drivers/gpu/drm/i915/intel_panel.c| 4 + 5 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_dp_aux_backlight.c diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 0851de07..41250cc 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -77,6 +77,7 @@ i915-y += dvo_ch7017.o \ dvo_tfp410.o \ intel_crt.o \ intel_ddi.o \ + intel_dp_aux_backlight.o \ intel_dp_link_training.o \ intel_dp_mst.o \ intel_dp.o \ diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a073f04..9f8672e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3183,7 +3183,7 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder) * Sinks are *supposed* to come up within 1ms from an off state, but we're also * supposed to retry 3 times per the spec. */ -static ssize_t +ssize_t intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset, void *buffer, size_t size) { @@ -3850,7 +3850,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct drm_device *dev = dig_port->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - uint8_t rev; if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp- dpcd, sizeof(intel_dp->dpcd)) < 0) @@ -3886,6 +3885,15 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.psr2_support ? "supported" : "not supported"); } + + /* Read the eDP Display control capabilities registers */ + memset(intel_dp->edp_dpcd, 0, sizeof(intel_dp- edp_dpcd)); + if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) && + (intel_dp_dpcd_read_wake(_dp->aux, DP_EDP_DPCD_REV, + intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) == + sizeof(intel_dp->edp_dpcd))) + DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) sizeof(intel_dp->edp_dpcd), + intel_dp->edp_dpcd); } DRM_DEBUG_KMS("Display Port TPS3 support: source %s, sink %s\n", @@ -3893,10 +3901,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) yesno(drm_dp_tps3_supported(intel_dp->dpcd))); /* Intermediate frequency support */ - if (is_edp(intel_dp) && - (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) && - (intel_dp_dpcd_read_wake(_dp->aux, DP_EDP_DPCD_REV, , 1) == 1) && - (rev >= 0x03)) { /* eDp v1.4 or higher */ + if (is_edp(intel_dp) && (intel_dp->edp_dpcd[0] >= 0x03)) { /* eDp +v1.4 or higher */ __le16 sink_rates[DP_MAX_SUPPORTED_RATES];
Re: [Intel-gfx] [PATCH V3] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
couple of questions since i am looking at SKL code for the first time > seems we are not reading max cd clock from VBIOS like BDW even though SKL has limit register to say max cd clock i dont think it is working, so VBIOS saves the value during boot just like in BDW and we are supposed to use it. please check VBT spec for the details > why should we store vco in a separate variable when it is already available as part of "pipe_config->dpll_hw_state.ctrl1" > still trying to understand the flow but is "ctrl1"/"VCO" in this patch written to DPLL_CTRL1 before we change the CD Clock ? if not then it might be a bug and must be fixed as part of changes here. regards, Sivakumar On 2/10/2016 5:58 AM, clinton.a.tay...@intel.com wrote: From: Clint TaylorTrack VCO frequency of SKL instead of the boot CDCLK and allow modeset to set cdclk based on the max required pixel clock based on VCO selected. The vco should be tracked at the atomic level and all CRTCs updated if the required vco is changed. At this time the eDP pll is configured inside the encoder which has no visibility into the atomic state. When eDP v1.4 panel that require the 8640 vco are available this may need to be investigated. V1: initial version V2: add vco tracking in intel_dp_compute_config(), rename skl_boot_cdclk. V3: rebase, V2 feedback not possible as encoders are not aware of atomic. Signed-off-by: Clint Taylor Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= --- drivers/gpu/drm/i915/i915_drv.h |2 +- drivers/gpu/drm/i915/intel_ddi.c |2 +- drivers/gpu/drm/i915/intel_display.c | 83 +- drivers/gpu/drm/i915/intel_dp.c |9 +++- drivers/gpu/drm/i915/intel_drv.h |1 + 5 files changed, 81 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8216665..f65dd1a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1822,7 +1822,7 @@ struct drm_i915_private { int num_fence_regs; /* 8 on pre-965, 16 otherwise */ unsigned int fsb_freq, mem_freq, is_ddr3; - unsigned int skl_boot_cdclk; + unsigned int skl_vco_freq; unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq; unsigned int max_dotclk_freq; unsigned int hpll_freq; diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 6d5b09f..285adab 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev) int cdclk_freq; cdclk_freq = dev_priv->display.get_display_clock_speed(dev); - dev_priv->skl_boot_cdclk = cdclk_freq; + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq); if (skl_sanitize_cdclk(dev_priv)) DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9e2273b..372a68f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq) return (freq - 1000) / 500; } -static unsigned int skl_cdclk_get_vco(unsigned int freq) +unsigned int skl_cdclk_get_vco(unsigned int freq) { unsigned int i; @@ -5821,17 +5821,17 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) void skl_init_cdclk(struct drm_i915_private *dev_priv) { - unsigned int required_vco; - /* DPLL0 not enabled (happens on early BIOS versions) */ if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) { /* enable DPLL0 */ - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk); - skl_dpll0_enable(dev_priv, required_vco); + if (dev_priv->skl_vco_freq != 8640) { + dev_priv->skl_vco_freq = 8100; + } + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq); } /* set CDCLK to the frequency the BIOS chose */ - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk); + skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500 : 308570 ); /* enable DBUF power */ I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST); @@ -5847,7 +5847,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) { uint32_t lcpll1 = I915_READ(LCPLL1_CTL); uint32_t cdctl = I915_READ(CDCLK_CTL); - int freq = dev_priv->skl_boot_cdclk; + int freq = dev_priv->cdclk_freq; /* * check if the pre-os intialized the display @@ -5871,11 +5871,7 @@ int skl_sanitize_cdclk(struct drm_i915_private
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add Backlight Control using DPCD for eDP connectors (v6)
On 2/5/2016 5:48 PM, Yetunde Adebisi wrote: This patch adds support for eDP backlight control using DPCD registers to backlight hooks in intel_panel. It checks for backlight control over AUX channel capability and sets up function pointers to get and set the backlight brightness level if supported. v2: Moved backlight functions from intel_dp.c into a new file intel_dp_aux_backlight.c. Also moved reading of eDP display control registers to intel_dp_get_dpcd v3: Correct some formatting mistakes v4: Updated to use AUX backlight control if PWM control is not possible (Jani) v5: Moved call to initialize backlight registers to dp_aux_setup_backlight v6: Check DP_EDP_BACKLIGHT_PIN_ENABLE_CAP is disabled before setting up AUX backlight control. To fix BLM_PWM_ENABLE igt test warnings on bdw_ultra This patch depends on http://patchwork.freedesktop.org/patch/64253/ Cc: Bob PaauweCc: Jani Nikula Acked-by: Jesse Barnes Signed-off-by: Yetunde Adebisi --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/intel_dp.c | 17 ++- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 170 ++ drivers/gpu/drm/i915/intel_drv.h | 6 + drivers/gpu/drm/i915/intel_panel.c| 4 + 5 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_dp_aux_backlight.c diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 0851de07..41250cc 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -77,6 +77,7 @@ i915-y += dvo_ch7017.o \ dvo_tfp410.o \ intel_crt.o \ intel_ddi.o \ + intel_dp_aux_backlight.o \ intel_dp_link_training.o \ intel_dp_mst.o \ intel_dp.o \ diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a073f04..9f8672e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3183,7 +3183,7 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder) * Sinks are *supposed* to come up within 1ms from an off state, but we're also * supposed to retry 3 times per the spec. */ -static ssize_t +ssize_t intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset, void *buffer, size_t size) { @@ -3850,7 +3850,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct drm_device *dev = dig_port->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - uint8_t rev; if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp->dpcd, sizeof(intel_dp->dpcd)) < 0) @@ -3886,6 +3885,15 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.psr2_support ? "supported" : "not supported"); } + + /* Read the eDP Display control capabilities registers */ + memset(intel_dp->edp_dpcd, 0, sizeof(intel_dp->edp_dpcd)); + if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) && + (intel_dp_dpcd_read_wake(_dp->aux, DP_EDP_DPCD_REV, + intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) == + sizeof(intel_dp->edp_dpcd))) + DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) sizeof(intel_dp->edp_dpcd), + intel_dp->edp_dpcd); } DRM_DEBUG_KMS("Display Port TPS3 support: source %s, sink %s\n", @@ -3893,10 +3901,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) yesno(drm_dp_tps3_supported(intel_dp->dpcd))); /* Intermediate frequency support */ - if (is_edp(intel_dp) && - (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) && - (intel_dp_dpcd_read_wake(_dp->aux, DP_EDP_DPCD_REV, , 1) == 1) && - (rev >= 0x03)) { /* eDp v1.4 or higher */ + if (is_edp(intel_dp) && (intel_dp->edp_dpcd[0] >= 0x03)) { /* eDp v1.4 or higher */ __le16 sink_rates[DP_MAX_SUPPORTED_RATES]; int i; diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c new file mode 100644 index 000..a5361d6 --- /dev/null +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -0,0 +1,170 @@ +/* + * Copyright © 2015 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without
Re: [Intel-gfx] [PATCH] drm/i915: Skip DDI PLL selection for DSI
On 2/5/2016 4:59 PM, Mika Kahola wrote: Skip DDI PLL selection if display type is DSI/MIPI. Signed-off-by: Mika Kahola--- drivers/gpu/drm/i915/intel_display.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d7de2a5..5da98b2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9902,8 +9902,13 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) static int haswell_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { - if (!intel_ddi_pll_select(crtc, crtc_state)) - return -EINVAL; + struct intel_encoder *intel_encoder = + intel_ddi_get_crtc_new_encoder(crtc_state); + + if (intel_encoder->type != INTEL_OUTPUT_DSI) { + if (!intel_ddi_pll_select(crtc, crtc_state)) + return -EINVAL; + } can this be moved inside bxt_ddi_pll_select ? we can avoid this check for other platforms that also execute this function. regards, Sivakumar crtc->lowfreq_avail = false; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: restrict PP save/restore to platforms with lvds
On 2/8/2016 3:25 PM, Daniel Vetter wrote: eDP already restores PP state completely on it's own, we only need this code for LVDS. Since it's more work to move this into the lvds encoder properly just limit it to affected pch chips for now (ibx/ppt). Cc: Jani NikulaAcked-by: Jani Nikula Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_suspend.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index a2aa09ce3202..7f6b050266a7 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev) else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) dev_priv->regfile.saveLVDS = I915_READ(LVDS); - /* Panel power sequencer */ - if (HAS_PCH_SPLIT(dev)) { + /* Panel power sequencer, only needed for LVDS */ + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL); dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS); dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS); won't this code execute for eDP too ? Also it seems incorrect we are saving and restoring PP_CONTROL register too when we should have followed proper sequence delays for each bit in PP_CONTROL. This will end up bring up the panel before modeset in best case or damage the panel in the worst case. Sivakumar @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev) else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask); - /* Panel power sequencer */ - if (HAS_PCH_SPLIT(dev)) { + /* Panel power sequencer, only needed for LVDS */ + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS); I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS); I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Skip DDI PLL selection for DSI
On 2/9/2016 12:02 PM, Jani Nikula wrote: On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasim...@intel.com> wrote: On 2/5/2016 4:59 PM, Mika Kahola wrote: Skip DDI PLL selection if display type is DSI/MIPI. Signed-off-by: Mika Kahola <mika.kah...@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d7de2a5..5da98b2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9902,8 +9902,13 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) static int haswell_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { - if (!intel_ddi_pll_select(crtc, crtc_state)) - return -EINVAL; + struct intel_encoder *intel_encoder = + intel_ddi_get_crtc_new_encoder(crtc_state); + + if (intel_encoder->type != INTEL_OUTPUT_DSI) { + if (!intel_ddi_pll_select(crtc, crtc_state)) + return -EINVAL; + } can this be moved inside bxt_ddi_pll_select ? we can avoid this check for other platforms that also execute this function. I asked Mika to do it this way, but if you feel strongly about it I guess I could be persuaded otherwise too. My main point is, if we pass on DSI encoders to DDI functions in some cases but mostly not, it will muddy the waters and eventually people end up checking for "is dsi" all around DDI just because they can't be bothered to check if the functions are really called for DDI only or not. It's more of a maintainability concern than anything else. BR, Jani. i am fine with this either way. i was thinking of avoid such checks in other platforms where it is not needed but your concern of too many is_dsi checks is valid as well. with that i am fine with this change as is. Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasim...@intel.com> regards, Sivakumar crtc->lowfreq_avail = false; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: restrict PP save/restore to platforms with lvds
On 2/9/2016 11:54 AM, Jani Nikula wrote: On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasim...@intel.com> wrote: On 2/8/2016 3:25 PM, Daniel Vetter wrote: eDP already restores PP state completely on it's own, we only need this code for LVDS. Since it's more work to move this into the lvds encoder properly just limit it to affected pch chips for now (ibx/ppt). Cc: Jani Nikula <jani.nik...@intel.com> Acked-by: Jani Nikula <jani.nik...@intel.com> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com> --- drivers/gpu/drm/i915/i915_suspend.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index a2aa09ce3202..7f6b050266a7 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -43,8 +43,8 @@ static void i915_save_display(struct drm_device *dev) else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) dev_priv->regfile.saveLVDS = I915_READ(LVDS); - /* Panel power sequencer */ - if (HAS_PCH_SPLIT(dev)) { + /* Panel power sequencer, only needed for LVDS */ + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL); dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS); dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS); won't this code execute for eDP too ? Also it seems incorrect we are saving and restoring PP_CONTROL register too when we should have followed proper sequence delays for each bit in PP_CONTROL. This will end up bring up the panel before modeset in best case or damage the panel in the worst case. Exactly the reasons why we are trying to limit this code to fewer platforms! This is very fragile stuff though, I fear we might regress something (even if that something is currently working by coincidence) if we limit this further, in one go. I'd like to do this little by little, and see what breaks, if anything. With this patch, we'd already limit all of this hackery to cougarpoint and older, instead of all PCH split up to and including Skylake. If anyone wants to dig into LVDS power sequencing after that, I'm not opposed... BR, Jani. missed the part where this patch is reducing the platforms :). can we reduce this to systems with LVDS alone ? i am sure eDP will blankout if pps is not followed but have observed some LVDS panels working even without pps delays. Sivakumar Sivakumar @@ -78,8 +78,8 @@ static void i915_restore_display(struct drm_device *dev) else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev)) I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask); - /* Panel power sequencer */ - if (HAS_PCH_SPLIT(dev)) { + /* Panel power sequencer, only needed for LVDS */ + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS); I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS); I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RESEND 1/5] drm/i915: move VBT based TV presence check to intel_bios.c
On 2/5/2016 4:06 PM, Jani Nikula wrote: Hide knowledge about VBT child devices in intel_bios.c. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_bios.c | 38 ++ drivers/gpu/drm/i915/intel_tv.c | 43 +-- 3 files changed, 40 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 77227a39f3d5..715f200cfbf5 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3397,6 +3397,7 @@ extern void intel_i2c_reset(struct drm_device *dev); /* intel_bios.c */ int intel_bios_init(struct drm_i915_private *dev_priv); bool intel_bios_is_valid_vbt(const void *buf, size_t size); +bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); /* intel_opregion.c */ #ifdef CONFIG_ACPI diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index bf62a19c8f69..2800ae50465a 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1431,3 +1431,41 @@ intel_bios_init(struct drm_i915_private *dev_priv) return 0; } + +/** + * intel_bios_is_tv_present - is integrated TV present in VBT + * @dev_priv: i915 device instance + * + * Return true if TV is present. If no child devices were parsed from VBT, + * assume TV is present. + */ +bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv) +{ + union child_device_config *p_child; + int i; + + if (!dev_priv->vbt.child_dev_num) + return true; + This is old code moved here, but should we return true when child_dev_num == 0 ? I understand we shouldn't make functional changes when moving code but just pointing out to check if this can be fixed in the further patches. + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { + p_child = dev_priv->vbt.child_dev + i; + /* +* If the device type is not TV, continue. +*/ + switch (p_child->old.device_type) { + case DEVICE_TYPE_INT_TV: + case DEVICE_TYPE_TV: + case DEVICE_TYPE_TV_SVIDEO_COMPOSITE: + break; + default: + continue; + } + /* Only when the addin_offset is non-zero, it is regarded +* as present. +*/ + if (p_child->old.addin_offset) + return true; + } + + return false; +} diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 5034b0055169..8417fcad02d2 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1530,47 +1530,6 @@ static const struct drm_encoder_funcs intel_tv_enc_funcs = { .destroy = intel_encoder_destroy, }; -/* - * Enumerate the child dev array parsed from VBT to check whether - * the integrated TV is present. - * If it is present, return 1. - * If it is not present, return false. - * If no child dev is parsed from VBT, it assumes that the TV is present. - */ -static int tv_is_present_in_vbt(struct drm_device *dev) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - union child_device_config *p_child; - int i, ret; - - if (!dev_priv->vbt.child_dev_num) - return 1; - - ret = 0; - for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { - p_child = dev_priv->vbt.child_dev + i; - /* -* If the device type is not TV, continue. -*/ - switch (p_child->old.device_type) { - case DEVICE_TYPE_INT_TV: - case DEVICE_TYPE_TV: - case DEVICE_TYPE_TV_SVIDEO_COMPOSITE: - break; - default: - continue; - } - /* Only when the addin_offset is non-zero, it is regarded -* as present. -*/ - if (p_child->old.addin_offset) { - ret = 1; - break; - } - } - return ret; -} - void intel_tv_init(struct drm_device *dev) { @@ -1586,7 +1545,7 @@ intel_tv_init(struct drm_device *dev) if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) return; - if (!tv_is_present_in_vbt(dev)) { + if (!intel_bios_is_tv_present(dev_priv)) { DRM_DEBUG_KMS("Integrated TV is not present.\n"); return; } saw below line immediately after the tv_is_present_in_vbt() call. can you move this inside the new function as well since we consider it as well before enumerating tv encoder. 1593 /* Even if we have an encoder we may not have a connector */ 1594 if
Re: [Intel-gfx] [PATCH RESEND 4/5] drm/i915: move VBT based DSI presence check to intel_bios.c
On 2/5/2016 4:06 PM, Jani Nikula wrote: Hide knowledge about VBT child devices in intel_bios.c. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_bios.c | 33 - drivers/gpu/drm/i915/intel_dsi.c | 23 ++- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 234f33708a31..cd2595c25f8d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1516,7 +1516,6 @@ struct intel_vbt_data { /* MIPI DSI */ struct { - u16 port; u16 panel_id; struct mipi_config *config; struct mipi_pps_data *pps; @@ -3400,6 +3399,7 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size); bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin); bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port); +bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port); /* intel_opregion.c */ #ifdef CONFIG_ACPI diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 1429d8214885..b0cf33234c33 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1237,7 +1237,6 @@ parse_device_mapping(struct drm_i915_private *dev_priv, &_child->common.device_type & DEVICE_TYPE_MIPI_OUTPUT) { DRM_DEBUG_KMS("Found MIPI as LFP\n"); dev_priv->vbt.has_mipi = 1; - dev_priv->vbt.dsi.port = p_child->common.dvo_port; } child_dev_ptr = dev_priv->vbt.child_dev + count; @@ -1552,3 +1551,35 @@ bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port) return false; } + +/** + * intel_bios_is_dsi_present - is DSI present in VBT + * @dev_priv: i915 device instance + * @port: port for DSI if present + * + * Return true if DSI is present, and return the port in %port. + */ +bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, + enum port *port) +{ + union child_device_config *p_child; + int i; + + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { + p_child = dev_priv->vbt.child_dev + i; + + if (!(p_child->common.device_type & DEVICE_TYPE_MIPI_OUTPUT)) + continue; + + switch (p_child->common.dvo_port) { + case DVO_PORT_MIPIA: + case DVO_PORT_MIPIB: + case DVO_PORT_MIPIC: + case DVO_PORT_MIPID: + *port = p_child->common.dvo_port - DVO_PORT_MIPIA; + return true; + } + } can we add platform check & vbt.has_mipi check ? it will avoid the need to loop through the array. Also since all platforms so far (VLV/CHV/BXT) has only MIPI A & MIPI C is B & D needed here ? we can directly fail here for unsupported ports rather than in dsi_init as special check. + + return false; +} diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 378f879f4015..5b6ec567e3ce 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -1112,9 +1112,15 @@ void intel_dsi_init(struct drm_device *dev) DRM_DEBUG_KMS("\n"); /* There is no detection method for MIPI so rely on VBT */ - if (!dev_priv->vbt.has_mipi) + if (!intel_bios_is_dsi_present(dev_priv, )) return; + if (port != PORT_A && port != PORT_C) { + DRM_DEBUG_KMS("VBT has unsupported DSI port %c\n", + port_name(port)); + return; + } + if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { dev_priv->mipi_mmio_base = VLV_MIPI_BASE; } else { @@ -1153,16 +1159,15 @@ void intel_dsi_init(struct drm_device *dev) intel_connector->unregister = intel_connector_unregister; /* Pipe A maps to MIPI DSI port A, pipe B maps to MIPI DSI port C */ - if (dev_priv->vbt.dsi.port == DVO_PORT_MIPIA) { - intel_encoder->crtc_mask = (1 << PIPE_A); - intel_dsi->ports = (1 << PORT_A); - } else if (dev_priv->vbt.dsi.port == DVO_PORT_MIPIC) { - intel_encoder->crtc_mask = (1 << PIPE_B); - intel_dsi->ports = (1 << PORT_C); - } + if (port == PORT_A) + intel_encoder->crtc_mask = 1 << PIPE_A; + else + intel_encoder->crtc_mask = 1 << PIPE_B; The above limitation applies only to CHT/VLV. if (dev_priv->vbt.dsi.config->dual_link) - intel_dsi->ports = ((1 << PORT_A) | (1 << PORT_C)); +
Re: [Intel-gfx] [PATCH RESEND 2/5] drm/i915: move VBT based LVDS presence check to intel_bios.c
On 2/5/2016 4:06 PM, Jani Nikula wrote: Hide knowledge about VBT child devices in intel_bios.c. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_bios.c | 50 drivers/gpu/drm/i915/intel_lvds.c | 53 +-- 3 files changed, 52 insertions(+), 52 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 715f200cfbf5..d70ef71d2538 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3398,6 +3398,7 @@ extern void intel_i2c_reset(struct drm_device *dev); int intel_bios_init(struct drm_i915_private *dev_priv); bool intel_bios_is_valid_vbt(const void *buf, size_t size); bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); +bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin); /* intel_opregion.c */ #ifdef CONFIG_ACPI diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 2800ae50465a..f3f4f8e687cf 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1469,3 +1469,53 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv) return false; } + +/** + * intel_bios_is_lvds_present - is LVDS present in VBT + * @dev_priv: i915 device instance + * @i2c_pin: i2c pin for LVDS if present + * + * Return true if LVDS is present. If no child devices were parsed from VBT, + * assume LVDS is present. + */ +bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin) +{ + int i; + + if (!dev_priv->vbt.child_dev_num) + return true; + we check if eDP is supported through following code before the call to this function. please add this check here so we can avoid vbt referencing for that. intel_lvds.c 972 if (HAS_PCH_SPLIT(dev)) { 973 if ((lvds & LVDS_DETECTED) == 0) 974 return; 975 if (dev_priv->vbt.edp_support) { 976 DRM_DEBUG_KMS("disable LVDS for eDP support\n"); 977 return; 978 } 979 } Sivakumar + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { + union child_device_config *uchild = dev_priv->vbt.child_dev + i; + struct old_child_dev_config *child = >old; + + /* If the device type is not LFP, continue. +* We have to check both the new identifiers as well as the +* old for compatibility with some BIOSes. +*/ + if (child->device_type != DEVICE_TYPE_INT_LFP && + child->device_type != DEVICE_TYPE_LFP) + continue; + + if (intel_gmbus_is_valid_pin(dev_priv, child->i2c_pin)) + *i2c_pin = child->i2c_pin; + + /* However, we cannot trust the BIOS writers to populate +* the VBT correctly. Since LVDS requires additional +* information from AIM blocks, a non-zero addin offset is +* a good indicator that the LVDS is actually present. +*/ + if (child->addin_offset) + return true; + + /* But even then some BIOS writers perform some black magic +* and instantiate the device without reference to any +* additional data. Trust that if the VBT was written into +* the OpRegion then they have validated the LVDS's existence. +*/ + if (dev_priv->opregion.vbt) + return true; + } + + return false; +} diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 0da0240caf81..80f8684e5137 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -772,57 +772,6 @@ static const struct dmi_system_id intel_no_lvds[] = { { } /* terminating entry */ }; -/* - * Enumerate the child dev array parsed from VBT to check whether - * the LVDS is present. - * If it is present, return 1. - * If it is not present, return false. - * If no child dev is parsed from VBT, it assumes that the LVDS is present. - */ -static bool lvds_is_present_in_vbt(struct drm_device *dev, - u8 *i2c_pin) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - int i; - - if (!dev_priv->vbt.child_dev_num) - return true; - - for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { - union child_device_config *uchild = dev_priv->vbt.child_dev + i; - struct old_child_dev_config *child = >old; - - /* If the device type is not LFP, continue. -* We have to check both the new identifiers as well as the -
Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: reduce missing TPS3 support errors to debug logging
Reviewed-by: Sivakumar ThulasimaniOn 2/5/2016 3:46 PM, Jani Nikula wrote: Per spec, TPS3 support is mandatory for downstream devices that support HBR2. We've therefore logged errors on HBR2 without TPS3 since commit 1da7d7131c35cde83f1bab8ec732b57b69bef814 Author: Jani Nikula Date: Thu Sep 3 11:16:08 2015 +0300 drm/i915: ignore link rate in TPS3 selection However, it seems there are real world devices out there that just aren't spec compliant, and still work at HBR2 using TPS2. So reduce the error message to debug logging. Cc: Ander Conselvan de Oliveira Cc: Sivakumar Thulasimani Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92932 Fixes: 1da7d7131c35 ("drm/i915: ignore link rate in TPS3 selection") Cc: drm-intel-fi...@lists.freedesktop.org Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_dp_link_training.c | 20 ++-- 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index 83e667b92fda..0b8eefc2acc5 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -222,19 +222,27 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) static u32 intel_dp_training_pattern(struct intel_dp *intel_dp) { u32 training_pattern = DP_TRAINING_PATTERN_2; + bool source_tps3, sink_tps3; /* * Intel platforms that support HBR2 also support TPS3. TPS3 support is -* also mandatory for downstream devices that support HBR2. +* also mandatory for downstream devices that support HBR2. However, not +* all sinks follow the spec. * * Due to WaDisableHBR2 SKL < B0 is the only exception where TPS3 is -* supported but still not enabled. +* supported in source but still not enabled. */ - if (intel_dp_source_supports_hbr2(intel_dp) && - drm_dp_tps3_supported(intel_dp->dpcd)) + source_tps3 = intel_dp_source_supports_hbr2(intel_dp); + sink_tps3 = drm_dp_tps3_supported(intel_dp->dpcd); + + if (source_tps3 && sink_tps3) { training_pattern = DP_TRAINING_PATTERN_3; - else if (intel_dp->link_rate == 54) - DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3 support\n"); + } else if (intel_dp->link_rate == 54) { + if (!source_tps3) + DRM_DEBUG_KMS("5.4 Gbps link rate without source HBR2/TPS3 support\n"); + if (!sink_tps3) + DRM_DEBUG_KMS("5.4 Gbps link rate without sink TPS3 support\n"); + } return training_pattern; } ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: abstract training pattern selection
Reviewed-by: Sivakumar ThulasimaniOn 2/5/2016 3:46 PM, Jani Nikula wrote: Make it cleaner to add more checks in the function. No functional changes. Cc: Ander Conselvan de Oliveira Cc: Sivakumar Thulasimani Cc: drm-intel-fi...@lists.freedesktop.org # dependency on the next patch Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_dp_link_training.c | 25 ++--- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index 7938e0bf..83e667b92fda 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -215,16 +215,15 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) } } -static void -intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) +/* + * Pick training pattern for channel equalization. Training Pattern 3 for HBR2 + * or 1.2 devices that support it, Training Pattern 2 otherwise. + */ +static u32 intel_dp_training_pattern(struct intel_dp *intel_dp) { - bool channel_eq = false; - int tries, cr_tries; - uint32_t training_pattern = DP_TRAINING_PATTERN_2; + u32 training_pattern = DP_TRAINING_PATTERN_2; /* -* Training Pattern 3 for HBR2 or 1.2 devices that support it. -* * Intel platforms that support HBR2 also support TPS3. TPS3 support is * also mandatory for downstream devices that support HBR2. * @@ -237,6 +236,18 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) else if (intel_dp->link_rate == 54) DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3 support\n"); + return training_pattern; +} + +static void +intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) +{ + bool channel_eq = false; + int tries, cr_tries; + u32 training_pattern; + + training_pattern = intel_dp_training_pattern(intel_dp); + /* channel equalization */ if (!intel_dp_set_link_train(intel_dp, training_pattern | ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix to allow render compression on primary plane of PIPEB
not sure how this can be pushed separately even if approved at present. why not push this as part of new patch set of the RC patches ? but ignoring the dependency this is fine. Reviewed-by: Sivakumar ThulasimaniOn 2/4/2016 11:44 AM, Mayuresh Gharpure wrote: Currently, a flip with render compression enabled is failing on primary plane of HDMI panel which is driven by PIPEB, this issue is fixed with this patch Change-Id: I197fe61faffad9da58733ed3f0e8cf6ef8640af7 Signed-off-by: Mayuresh Gharpure --- Please note that this patch depends on following patch: https://patchwork.freedesktop.org/patch/67448/ Current patch is a bug fix on the above mentioned patch drivers/gpu/drm/i915/intel_display.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4b23ec2..f8485fa 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12777,10 +12777,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, if (fb && to_intel_plane_state(plane_state)-> render_comp_enable) { - if (to_intel_plane(plane)->plane != PLANE_A) { - DRM_DEBUG_KMS("RC supported only on planes 1 & 2\n"); - return -EINVAL; - } ret = skl_check_compression(dev, to_intel_plane_state(plane_state), intel_crtc->pipe, crtc->x, crtc->y); @@ -15088,6 +15084,7 @@ static int skl_check_compression(struct drm_device *dev, enum pipe pipe, int x, int y) { struct drm_framebuffer *fb = plane_state->base.fb; + struct drm_plane *plane = plane_state->base.plane; int x_offset; int src_w = drm_rect_width(_state->src) >> 16; @@ -15127,6 +15124,13 @@ static int skl_check_compression(struct drm_device *dev, return -EINVAL; } + if (!(plane->type == DRM_PLANE_TYPE_PRIMARY || + (plane->type == DRM_PLANE_TYPE_OVERLAY && + to_intel_plane(plane)->plane == PLANE_A))) { + DRM_DEBUG_KMS("RC supported only on planes 1 & 2\n"); + return -EINVAL; + } + if (intel_rotation_90_or_270(plane_state->base.rotation)) { DRM_DEBUG_KMS("RC support only with 0/180 degree rotation\n"); return -EINVAL; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Set invert bit for hpd based on VBT
On 2/4/2016 5:59 PM, Jani Nikula wrote: On Thu, 04 Feb 2016, Shubhangi Shrivastavawrote: This patch sets the invert bit for hpd detection for each port based on vbt configuration. since each AOB can be designed to depend on invert bit or not, it is expected if an AOB requires invert bit, the user will set respective bit in VBT. Signed-off-by: Sivakumar Thulasimani Signed-off-by: Durgadoss R Signed-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/i915_irq.c | 49 + drivers/gpu/drm/i915/i915_reg.h | 9 2 files changed, 58 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 25a8937..305e6dd 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3424,6 +3424,54 @@ static void ibx_hpd_irq_setup(struct drm_device *dev) I915_WRITE(PCH_PORT_HOTPLUG, hotplug); } +/* + * For BXT invert bit has to be set based on AOB design + * for HPD detection logic, update it based on VBT fields. + */ +static void bxt_hpd_set_invert(struct drm_device *dev, u32 hotplug_port) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int i, reg_val, val = 0; + + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { + + /* Proceed only if invert bit is set */ + if (dev_priv->vbt.child_dev[i].common.hpd_invert == 0) + continue; + + /* +* Convert dvo_port to PORT_X and set appropriate bit +* only if hotplug is enabled on that port +*/ + switch (dev_priv->vbt.child_dev[i].common.dvo_port) { + case DVO_PORT_DPA: + case DVO_PORT_HDMIA: + if (hotplug_port & BXT_DE_PORT_HP_DDIA) + val |= BXT_DDIA_HPD_INVERT; + break; + case DVO_PORT_DPB: + case DVO_PORT_HDMIB: + if (hotplug_port & BXT_DE_PORT_HP_DDIB) + val |= BXT_DDIB_HPD_INVERT; + break; + case DVO_PORT_DPC: + case DVO_PORT_HDMIC: + if (hotplug_port & BXT_DE_PORT_HP_DDIC) + val |= BXT_DDIC_HPD_INVERT; + break; + default: + DRM_ERROR("HPD invert set for invalid dvo port %d\n", + dev_priv->vbt.child_dev[i].common.dvo_port); + break; + } + } + reg_val = I915_READ(BXT_HOTPLUG_CTL); + DRM_DEBUG_KMS("Invert bit setting: hp_ctl:%x hp_port:%x val:%x\n", + reg_val, hotplug_port, val); + reg_val &= ~BXT_DDI_HPD_INVERT_MASK; + I915_WRITE(BXT_HOTPLUG_CTL, reg_val | val); +} No, we don't want this here. Separate VBT parsing from the rest of the logic. See [1] for some directions where I want to take this type of things. hmm understood, will add intel_bios_requires_invert(dev, port) and change the logic above to if (intel_bios_requires_invert(dev,port) val |= port; hope this should be fine. BR, Jani. [1] http://mid.gmane.org/cover.1452541881.git.jani.nik...@intel.com + static void spt_hpd_irq_setup(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3494,6 +3542,7 @@ static void bxt_hpd_irq_setup(struct drm_device *dev) hotplug |= PORTC_HOTPLUG_ENABLE | PORTB_HOTPLUG_ENABLE | PORTA_HOTPLUG_ENABLE; I915_WRITE(PCH_PORT_HOTPLUG, hotplug); + bxt_hpd_set_invert(dev, enabled_irqs); } static void ibx_irq_postinstall(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0a98889..01bd3c5 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5936,6 +5936,15 @@ enum skl_disp_power_wells { #define GEN8_PCU_IIR _MMIO(0x444e8) #define GEN8_PCU_IER _MMIO(0x444ec) +/* BXT hotplug control */ +#define BXT_HOTPLUG_CTL_MMIO(0xC4030) +#define BXT_DDIA_HPD_INVERT(1 << 27) +#define BXT_DDIC_HPD_INVERT(1 << 11) +#define BXT_DDIB_HPD_INVERT(1 << 3) +#define BXT_DDI_HPD_INVERT_MASK(BXT_DDIA_HPD_INVERT | \ +BXT_DDIB_HPD_INVERT | \ +BXT_DDIC_HPD_INVERT) + #define ILK_DISPLAY_CHICKEN2 _MMIO(0x42004) /* Required on all Ironlake and Sandybridge according to the B-Spec. */ #define ILK_ELPIN_409_SELECT (1 << 25) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bxt: Save/restore HOTPLUG_CTL during suspend/resume.
On 2/4/2016 7:21 AM, Matt Roper wrote: On Thu, Feb 04, 2016 at 07:17:08AM +0530, Thulasimani, Sivakumar wrote: On 2/4/2016 6:19 AM, Matt Roper wrote: From: Bob Paauwe <bob.j.paa...@intel.com> Broxton has some additional bits in the HOTPLUG_CTL register that indicate whether the HPD sense lines need to be inverted or not for the current platform. The BIOS sets these bits to an appropriate value at boot time, but the value is lost across suspend/resume. We need to save and restore the register so that hotplug and display detect works on resume. i have a patch that is about to be upstreamed that will read and write these values based on vbt. Shuhangi did the basic testing last week so will ask her to send to mail list today. i would prefer that patch where we know how and when to set these bits rather than just save & restore. Sure, sounds good. Can your patch handle cases where there is no VBT by falling back to a save/restore? Quite often in the embedded world, we have very specialized boot firmware that doesn't resemble vbios/gop and doesn't have any VBT info. hmm.. falling back will be required only when older VBTs are used. in which case we assume that invert bit is not needed. if you still need this patch then can you save and restore only the invert bits ? the code below does it for whole register and might enable interrupts too which is not intention of the patch. regards, Sivakumar Matt regards, Sivakumar Signed-off-by: Bob Paauwe <bob.j.paa...@intel.com> [mattrope: Expand commit message] Signed-off-by: Matt Roper <matthew.d.ro...@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_suspend.c | 7 +++ 2 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 77227a3..2278117 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1061,6 +1061,7 @@ struct i915_suspend_saved_registers { uint64_t saveFENCE[I915_MAX_NUM_FENCES]; u32 savePCH_PORT_HOTPLUG; u16 saveGCDGMBUS; + u32 saveHOTPLUG; }; struct vlv_s0ix_state { diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index 34e061a..efe1e77 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -59,6 +59,10 @@ static void i915_save_display(struct drm_device *dev) /* save FBC interval */ if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) dev_priv->regfile.saveFBC_CONTROL = I915_READ(FBC_CONTROL); + + if (IS_BROXTON(dev)) + dev_priv->regfile.saveHOTPLUG = I915_READ(PCH_PORT_HOTPLUG); + } static void i915_restore_display(struct drm_device *dev) @@ -98,6 +102,9 @@ static void i915_restore_display(struct drm_device *dev) if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) I915_WRITE(FBC_CONTROL, dev_priv->regfile.saveFBC_CONTROL); + if (IS_BROXTON(dev)) + I915_WRITE(PCH_PORT_HOTPLUG, dev_priv->regfile.saveHOTPLUG); + i915_redisable_vga(dev); } ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bxt: Save/restore HOTPLUG_CTL during suspend/resume.
On 2/4/2016 7:29 AM, Thulasimani, Sivakumar wrote: On 2/4/2016 7:21 AM, Matt Roper wrote: On Thu, Feb 04, 2016 at 07:17:08AM +0530, Thulasimani, Sivakumar wrote: On 2/4/2016 6:19 AM, Matt Roper wrote: From: Bob Paauwe <bob.j.paa...@intel.com> Broxton has some additional bits in the HOTPLUG_CTL register that indicate whether the HPD sense lines need to be inverted or not for the current platform. The BIOS sets these bits to an appropriate value at boot time, but the value is lost across suspend/resume. We need to save and restore the register so that hotplug and display detect works on resume. i have a patch that is about to be upstreamed that will read and write these values based on vbt. Shuhangi did the basic testing last week so will ask her to send to mail list today. i would prefer that patch where we know how and when to set these bits rather than just save & restore. Sure, sounds good. Can your patch handle cases where there is no VBT by falling back to a save/restore? Quite often in the embedded world, we have very specialized boot firmware that doesn't resemble vbios/gop and doesn't have any VBT info. hmm.. falling back will be required only when older VBTs are used. in which case we assume that invert bit is not needed. if you still need this patch then can you save and restore only the invert bits ? the code below does it for whole register and might enable interrupts too which is not intention of the patch. i am a bit more concerned on the scenario where there is no GOP/VBIOS and so no VBT. can you share how common it is not have VBIOS/GOP ? i am aware of coreboot but not sure if it has VBT or not (will check on this). VBT is currently used in nightly where we have no other options but whenever we dont have to depend on it we have some sort if WA. i feel this is not good since we cannot say if some scenario will work or not if VBT is not present. may be a new thread just to discuss that ? regards, Sivakumar Matt regards, Sivakumar Signed-off-by: Bob Paauwe <bob.j.paa...@intel.com> [mattrope: Expand commit message] Signed-off-by: Matt Roper <matthew.d.ro...@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_suspend.c | 7 +++ 2 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 77227a3..2278117 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1061,6 +1061,7 @@ struct i915_suspend_saved_registers { uint64_t saveFENCE[I915_MAX_NUM_FENCES]; u32 savePCH_PORT_HOTPLUG; u16 saveGCDGMBUS; +u32 saveHOTPLUG; }; struct vlv_s0ix_state { diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index 34e061a..efe1e77 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -59,6 +59,10 @@ static void i915_save_display(struct drm_device *dev) /* save FBC interval */ if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) dev_priv->regfile.saveFBC_CONTROL = I915_READ(FBC_CONTROL); + +if (IS_BROXTON(dev)) +dev_priv->regfile.saveHOTPLUG = I915_READ(PCH_PORT_HOTPLUG); + } static void i915_restore_display(struct drm_device *dev) @@ -98,6 +102,9 @@ static void i915_restore_display(struct drm_device *dev) if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) I915_WRITE(FBC_CONTROL, dev_priv->regfile.saveFBC_CONTROL); +if (IS_BROXTON(dev)) +I915_WRITE(PCH_PORT_HOTPLUG, dev_priv->regfile.saveHOTPLUG); + i915_redisable_vga(dev); } ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bxt: Save/restore HOTPLUG_CTL during suspend/resume.
On 2/4/2016 6:19 AM, Matt Roper wrote: From: Bob PaauweBroxton has some additional bits in the HOTPLUG_CTL register that indicate whether the HPD sense lines need to be inverted or not for the current platform. The BIOS sets these bits to an appropriate value at boot time, but the value is lost across suspend/resume. We need to save and restore the register so that hotplug and display detect works on resume. i have a patch that is about to be upstreamed that will read and write these values based on vbt. Shuhangi did the basic testing last week so will ask her to send to mail list today. i would prefer that patch where we know how and when to set these bits rather than just save & restore. regards, Sivakumar Signed-off-by: Bob Paauwe [mattrope: Expand commit message] Signed-off-by: Matt Roper --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_suspend.c | 7 +++ 2 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 77227a3..2278117 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1061,6 +1061,7 @@ struct i915_suspend_saved_registers { uint64_t saveFENCE[I915_MAX_NUM_FENCES]; u32 savePCH_PORT_HOTPLUG; u16 saveGCDGMBUS; + u32 saveHOTPLUG; }; struct vlv_s0ix_state { diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index 34e061a..efe1e77 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -59,6 +59,10 @@ static void i915_save_display(struct drm_device *dev) /* save FBC interval */ if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) dev_priv->regfile.saveFBC_CONTROL = I915_READ(FBC_CONTROL); + + if (IS_BROXTON(dev)) + dev_priv->regfile.saveHOTPLUG = I915_READ(PCH_PORT_HOTPLUG); + } static void i915_restore_display(struct drm_device *dev) @@ -98,6 +102,9 @@ static void i915_restore_display(struct drm_device *dev) if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) I915_WRITE(FBC_CONTROL, dev_priv->regfile.saveFBC_CONTROL); + if (IS_BROXTON(dev)) + I915_WRITE(PCH_PORT_HOTPLUG, dev_priv->regfile.saveHOTPLUG); + i915_redisable_vga(dev); } ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/dsi: Add audio reference in dsi encoder
Again like previous patch can you add checks for IS_BROXTON for all of these changes so it does not affect CHV/VLV ? regards, Sivakumar On 2/2/2016 11:24 PM, Ramalingam C wrote: From: "Kumar, Mahesh"We are re-using Mipi encoder enabled by GOP driver, but not incrementing reference count for Audio Power domain, so audio was not working. This patch increments the reference count during DSI init and Adds get/put in DSI enable/disable functions as well so audio power domain will be on when mipi is in use. Signed-off-by: Kumar, Mahesh Signed-off-by: Uma Shankar Signed-off-by: Ramalingam C --- drivers/gpu/drm/i915/intel_dsi.c | 13 + 1 file changed, 13 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 91cef35..8b43ef6 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -461,6 +461,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) intel_dsi_port_enable(encoder); } + intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); + intel_audio_codec_enable(encoder); + intel_panel_enable_backlight(intel_dsi->attached_connector); } @@ -531,11 +534,16 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder) static void intel_dsi_pre_disable(struct intel_encoder *encoder) { + struct drm_device *dev = encoder->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base); enum port port; DRM_DEBUG_KMS("\n"); + intel_audio_codec_disable(encoder); + intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO); + intel_panel_disable_backlight(intel_dsi->attached_connector); if (is_vid_mode(intel_dsi)) { @@ -1236,6 +1244,11 @@ void intel_dsi_init(struct drm_device *dev) intel_panel_init(_connector->panel, fixed_mode, NULL); intel_panel_setup_backlight(connector, INVALID_PIPE); + /* Enable Audio Power to fix use-count state machine */ + port = (dev_priv->vbt.dsi.port == DVO_PORT_MIPIA) ? PORT_A : PORT_C; + if (I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE) + intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); + return; err: ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/BXT: Fixed COS blanking issue
On 2/2/2016 11:24 PM, Ramalingam C wrote: From: Uma ShankarDuring Charging OS mode, mipi display was blanking.This is because during driver load, though encoder, connector were active but crtc returned inactive. This caused sanitize function to disable the DSI panel. In AOS, this is fine since HWC will do a modeset and crtc, connector, encoder mapping will be restored. But in COS, no modeset is called, it just calls DPMS ON/OFF. This is fine on BYT/CHT since transcoder is common b/w all encoders. But for BXT, there is a separate mipi transcoder. Hence this needs special handling for BXT. Signed-off-by: Uma Shankar Signed-off-by: Ramalingam C --- drivers/gpu/drm/i915/intel_display.c | 107 -- 1 file changed, 101 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a66220a..f8685f5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7814,6 +7814,69 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc) (intel_crtc->config->pipe_src_h - 1)); } +static void intel_get_dsi_pipe_timings(struct intel_crtc *crtc, + struct intel_crtc_state *pipe_config) +{ + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + enum transcoder cpu_transcoder = pipe_config->cpu_transcoder; + struct intel_encoder *encoder; + uint32_t tmp; + + tmp = I915_READ(HTOTAL(cpu_transcoder)); + pipe_config->base.adjusted_mode.crtc_hdisplay = (tmp & 0x) + 1; + pipe_config->base.adjusted_mode.crtc_htotal = + ((tmp >> 16) & 0x) + 1; + tmp = I915_READ(HBLANK(cpu_transcoder)); + pipe_config->base.adjusted_mode.crtc_hblank_start = (tmp & 0x) + 1; + pipe_config->base.adjusted_mode.crtc_hblank_end = + ((tmp >> 16) & 0x) + 1; + tmp = I915_READ(HSYNC(cpu_transcoder)); + pipe_config->base.adjusted_mode.crtc_hsync_start = (tmp & 0x) + 1; + pipe_config->base.adjusted_mode.crtc_hsync_end = + ((tmp >> 16) & 0x) + 1; + + tmp = I915_READ(VBLANK(cpu_transcoder)); + pipe_config->base.adjusted_mode.crtc_vblank_start = (tmp & 0x) + 1; + pipe_config->base.adjusted_mode.crtc_vblank_end = + ((tmp >> 16) & 0x) + 1; + tmp = I915_READ(VSYNC(cpu_transcoder)); + pipe_config->base.adjusted_mode.crtc_vsync_start = (tmp & 0x) + 1; + pipe_config->base.adjusted_mode.crtc_vsync_end = + ((tmp >> 16) & 0x) + 1; + + if (I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_INTERLACE_MASK) { + pipe_config->base.adjusted_mode.flags |= + DRM_MODE_FLAG_INTERLACE; + pipe_config->base.adjusted_mode.crtc_vtotal += 1; + pipe_config->base.adjusted_mode.crtc_vblank_end += 1; + } + + + for_each_encoder_on_crtc(dev, >base, encoder) { + struct intel_dsi *intel_dsi = + enc_to_intel_dsi(>base); + enum port port; + + pipe_config->pipe_bpp = intel_dsi->dsi_bpp; + for_each_dsi_port(port, intel_dsi->ports) { + pipe_config->base.adjusted_mode.crtc_hdisplay = + I915_READ(BXT_MIPI_TRANS_HACTIVE(port)); + pipe_config->base.adjusted_mode.crtc_vdisplay = + I915_READ(BXT_MIPI_TRANS_VACTIVE(port)); + pipe_config->base.adjusted_mode.crtc_vtotal = + I915_READ(BXT_MIPI_TRANS_VTOTAL(port)); + } + } + + tmp = I915_READ(PIPESRC(crtc->pipe)); + pipe_config->pipe_src_h = (tmp & 0x) + 1; + pipe_config->pipe_src_w = ((tmp >> 16) & 0x) + 1; + + pipe_config->base.mode.vdisplay = pipe_config->pipe_src_h; + pipe_config->base.mode.hdisplay = pipe_config->pipe_src_w; +} + static void intel_get_pipe_timings(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { @@ -9969,6 +10032,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, struct drm_i915_private *dev_priv = dev->dev_private; enum intel_display_power_domain pfit_domain; uint32_t tmp; + bool is_dsi = false; if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(crtc->pipe))) @@ -,17 +10063,48 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
Re: [Intel-gfx] [PATCH] drm/i915/BXT: Configure DSI after enabling DSI pll
just realized that intel_dsi_init is not called from setup outputs for BXT. is this expected ? if so when is it expected to be added ? Again, the current code in intel_setup_outputs calls intel_dsi_init from vlv/chv section so please confirm if this is needed for all platforms or just in BXT. On 2/2/2016 11:21 PM, Ramalingam C wrote: We need to enable DSI PLL before configuring the DSI registers. Signed-off-by: Ramalingam C--- drivers/gpu/drm/i915/intel_dsi.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 91cef35..378f879 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -478,8 +478,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder) DRM_DEBUG_KMS("\n"); - intel_dsi_prepare(encoder); intel_enable_dsi_pll(encoder); + intel_dsi_prepare(encoder); /* Panel Enable over CRC PMIC */ if (intel_dsi->gpio_panel) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse
On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote: On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote: On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote: On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: Current DP detection has DPCD operations split across intel_dp_hpd_pulse and intel_dp_detect which contains duplicates as well. Also intel_dp_detect is called during modes enumeration as well which will result in multiple dpcd operations. So this patch tries to solve both these by bringing all DPCD operations in one single function and make intel_dp_detect use existing values instead of repeating same steps. v2: Pulled in a hunk from last patch of the series to this patch. (Ander) v3: Added MST hotplug handling. (Ander) Tested-by: Nathan D CiobanuSigned-off-by: Sivakumar Thulasimani Signed-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/intel_dp.c | 71 +-- - - 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 8969ff9..82ee18d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c [...] @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool force) return connector_status_disconnected; } - intel_dp_long_pulse(intel_dp->attached_connector); + if (force) + intel_dp_long_pulse(intel_dp->attached_connector); I didn't notice this at first, but force is not the right thing to check for here. It is basically intended to avoid doing load detection (see intel_get_load_detect_pipe()) on automated polling. But we still have to try detection here when force = false, otherwise this will cause a regression. If you plug in a DP device while suspended, that device won't get detected, since we don't get an HPD for it. Previously, the call do intel_dp_detect() with force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would cause a full detection. To avoid the repeated DPCD operations, I think we need a more explicit mechanism to signal that we already handled the long pulse via the HPD handler. In intel_dp_hpd_pulse() we could set a flag that tells we just handled a long pulse for the given port. The call to intel_dp_long_pulse() in intel_dp_detect() would then be dependent on that flag. For that reason I have to retract my R-b from this patch. Ander Call to intel_dp_detect() from get_modes is with force set to true while from resume the call is with force set to false.. It should be in the opposite manner as get_modes should not require full detection whereas resume should. So, this needs to be cleaned up there. After merge of these patches, will look into cleaning up that part of the code. That really depends on what the force parameter is supposed to mean. The documentation states that "force is set to false whilst polling, true when checking the connector due to a user request". A look through git history shows the parameter was added to reduce time wasted doing load detection (doing a mode set in order to check if there is a device connected) for hardware that needs it (commit 7b334fcb45b7). As far as I can tell, across all the drm drivers, that parameter is only used to avoid doing load detection. Another thing to consider is that the driver may switch to polling if it detects an HPD storm. When the detect calls come from polling, the code in this patch will simply avoid any detection. hmm i think this discussion will prolong for a while :) how about we call intel_dp_long_pulse() always for now. this will be a compromise to not break any of the existing code but will still result in getting a clean detection code, which will can then be improved upon in the next iteration ? i.e post the change it should look like. i.e skip this change alone intel_dp_long_pulse(intel_dp->attached_connector); regards, Sivakumar Moreover, intel_dp_detect() will be called from drm_helper_hpd_irq_event() in polling scenarios only (when DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like this code here, doesn't really create a regression for realtime scenarios. I don't know what you mean by realtime scenarios, but the regression is very real. Using a kernel with your patches applied, suspend while there is no DP monitor attached, attach the monitor while suspended and then wake up. Notice how the connector state doesn't change. You can check the i915_display_info file in debugfs, for instance. Ander if (intel_connector->detect_edid) return connector_status_connected; @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) /* indicate that we need to restart link training */
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Sanity check DP AUX message buffer and size
On 1/29/2016 6:22 PM, Imre Deak wrote: While we are calling intel_dp_aux_transfer() with msg->size=0 whenever msg->buffer is NULL, passing NULL to memcpy() is undefined according to the ISO C standard. I haven't found any notes about this in the GNU C's or the kernel's documentation of the function and can't imagine what it would do with the NULL ptr. To better document this use of the parameters it still make sense to add an explicit check for this to the code. Caught by Coverity. can you share more info on when is this scenario triggered ? Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_dp.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e2bea710..2aed36e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -979,7 +979,10 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) if (WARN_ON(txsize > 20)) return -E2BIG; - memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size); + if (msg->buffer) + memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size); + else + WARN_ON(msg->size); ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize); if (ret > 0) { ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Instrument PSR parameter for possible quirks with link standby.
On 1/21/2016 5:26 PM, Zanoni, Paulo R wrote: Em Qui, 2016-01-21 às 08:35 +0530, Thulasimani, Sivakumar escreveu: On 1/20/2016 10:32 PM, Zanoni, Paulo R wrote: Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu: Unfortunately we don't know all panels and platforms out there and we found internal prototypes without VBT proper set but where only link in standby worked well. :) if it is internal i assume someone has to set the vbt ,we encountered an issue sometime back that blamed vbt as incorrect only to later learn that the person who created the setup didn't care to configure the VBT. But in order to discover if the VBT is incorrect, the parameter can be used. So, before enable PSR by default let's instrument the PSR parameter in a way that we can identify different panels out there that might require or work better with link standby mode. It is also useful to say that for backward compatibility I'm not changing the meaning of this flag. So "0" still means disabled and "1" means enabled with full support and maximum power savings. v2: Use positive value instead of negative for different operation mode as suggested by Daniel. v3: As Paulo suggested use 2 to force link standby and 3 to force link fully on. Also split the link_standby introduction in a separated patch. Cc: Paulo Zanoni <paulo.r.zan...@intel.com> Cc: Daniel Vetter <daniel.vet...@ffwll.ch> Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com> --- drivers/gpu/drm/i915/i915_params.c | 7 ++- drivers/gpu/drm/i915/intel_psr.c | 17 + 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 835d609..f78ddf3 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -126,7 +126,12 @@ MODULE_PARM_DESC(enable_execlists, "(-1=auto [default], 0=disabled, 1=enabled)"); module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600); -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)"); +MODULE_PARM_DESC(enable_psr, "Enable PSR " +"(0=disabled [default], 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)" +"In case you needed to force any different option, please " +"report PCI device ID, subsystem vendor and subsystem device ID " +"to intel-gfx@lists.freedesktop.org, if your machine needs it. " +"It will then be included in an upcoming module version."); Are we making a promise here? Isn't that dangerous? :P I'd just tell the users to open bug reports. (I'm not requiring you to change anything here, but something something lawyers something) module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600); MODULE_PARM_DESC(preliminary_hw_support, diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index b84ec80..c3c2bb8 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -335,6 +335,12 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp) return false; } + if ((IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) && + dev_priv->psr.link_standby) { IS_VALLEYVIEW() will return true for both valleyview and cherryview so the above check for cherryview can be removed. Not anymore: commit 666a45379e2c29bc16e60648e5ad8f6f8b7fa6ce Author: Wayne Boyer <wayne.bo...@intel.com> Date: Wed Dec 9 12:29:35 2015 -0800 drm/i915: Separate cherryview from valleyview Thanks for commit id of patch :). jumping on upstream every now and then will lead to missing a lot of changes happening. s/dev_priv->psr.link_standby/!dev_priv->psr.link_standby/ Also, I'm not sure if this chunk belongs here or at intel_psr_init(), since it effectively disables PSR. This means that i915.enable_psr=3 disables PSR on VLV/CHV. But maybe we shouldn't care since users shouldn't be using the option anyway. On the other hand, users may start claiming that i915.enable_psr=X "fixed PSR" for them while effectively it just disabled PSR, so perhaps DRM_ERROR would be better. Anyway, I'm not requesting any change, just pointing things in case you or someone else has any idea, but maybe I'd go with DRM_ERROR since users usually don't know which platform supports what, so the loud message may help them. i agree, psr_match_conditions should check for parameters that can change dynamically post boot to decide if we can enable psr or not, if link_standby cannot be changed post boot we should check for it in init so we can avoid psr being enabled in the first place. Another check which we seem to be missing is "if (HAS_DDI(dev
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Instrument PSR parameter for possible quirks with link standby.
On 1/20/2016 10:32 PM, Zanoni, Paulo R wrote: Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu: Unfortunately we don't know all panels and platforms out there and we found internal prototypes without VBT proper set but where only link in standby worked well. :) if it is internal i assume someone has to set the vbt ,we encountered an issue sometime back that blamed vbt as incorrect only to later learn that the person who created the setup didn't care to configure the VBT. So, before enable PSR by default let's instrument the PSR parameter in a way that we can identify different panels out there that might require or work better with link standby mode. It is also useful to say that for backward compatibility I'm not changing the meaning of this flag. So "0" still means disabled and "1" means enabled with full support and maximum power savings. v2: Use positive value instead of negative for different operation mode as suggested by Daniel. v3: As Paulo suggested use 2 to force link standby and 3 to force link fully on. Also split the link_standby introduction in a separated patch. Cc: Paulo ZanoniCc: Daniel Vetter Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/i915_params.c | 7 ++- drivers/gpu/drm/i915/intel_psr.c | 17 + 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 835d609..f78ddf3 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -126,7 +126,12 @@ MODULE_PARM_DESC(enable_execlists, "(-1=auto [default], 0=disabled, 1=enabled)"); module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600); -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)"); +MODULE_PARM_DESC(enable_psr, "Enable PSR " +"(0=disabled [default], 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)" +"In case you needed to force any different option, please " +"report PCI device ID, subsystem vendor and subsystem device ID " +"to intel-gfx@lists.freedesktop.org, if your machine needs it. " +"It will then be included in an upcoming module version."); Are we making a promise here? Isn't that dangerous? :P I'd just tell the users to open bug reports. (I'm not requiring you to change anything here, but something something lawyers something) module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600); MODULE_PARM_DESC(preliminary_hw_support, diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index b84ec80..c3c2bb8 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -335,6 +335,12 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp) return false; } + if ((IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) && + dev_priv->psr.link_standby) { IS_VALLEYVIEW() will return true for both valleyview and cherryview so the above check for cherryview can be removed. s/dev_priv->psr.link_standby/!dev_priv->psr.link_standby/ Also, I'm not sure if this chunk belongs here or at intel_psr_init(), since it effectively disables PSR. This means that i915.enable_psr=3 disables PSR on VLV/CHV. But maybe we shouldn't care since users shouldn't be using the option anyway. On the other hand, users may start claiming that i915.enable_psr=X "fixed PSR" for them while effectively it just disabled PSR, so perhaps DRM_ERROR would be better. Anyway, I'm not requesting any change, just pointing things in case you or someone else has any idea, but maybe I'd go with DRM_ERROR since users usually don't know which platform supports what, so the loud message may help them. i agree, psr_match_conditions should check for parameters that can change dynamically post boot to decide if we can enable psr or not, if link_standby cannot be changed post boot we should check for it in init so we can avoid psr being enabled in the first place. Another check which we seem to be missing is "if (HAS_DDI(dev_priv) && transcoder != TRANSCODER_EDP && !dev_priv->psr.link_standby)", but this depends on the result of the discussion of patch 1. Everything else looks good, but it would be nice to see the opinions of maintainers here since they always have something to say about new i915.ko options. + DRM_DEBUG_KMS("PSR condition failed: Link off requested/needed but not supported on this platform\n"); + return false; + } + sorry i came late to this thread, but can you point me to some issues for link off in CHT/VLV ? we have enabled link off in CHT Android and it seems to be working fine. we can check again if we have missed something. if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
Re: [Intel-gfx] [PATCH] drm/i915: Splitting intel_dp_check_link_status
On 1/19/2016 2:14 PM, Daniel Vetter wrote: On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote: On 1/19/2016 2:35 AM, Lukas Wunner wrote: Hi, On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote: When created originally intel_dp_check_link_status() was supposed to handle only link training for short pulse but has grown into handler for short pulse itself. This patch cleans up this function by splitting it into two halves. First intel_dp_short_pulse() is called, which will be entry point and handle all logic for short pulse handling while intel_dp_check_link_status() will retain its original purpose of only doing link status related work. The link retraining part when EQ is not correct is retained to intel_dp_check_link_status whereas other operations are handled as part of intel_dp_short_pulse. This change is required to avoid performing all DPCD related operations on performing link retraining. v2: Added WARN_ON to intel_dp_check_link_status() Removed a call to intel_dp_get_link_status() (Ander) Tested-by: Nathan D Ciobanu <nathan.d.ciob...@intel.com> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasim...@intel.com> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivast...@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 65 +++-- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 82ee18d..f8d9611 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4279,6 +4279,36 @@ go_again: return -EINVAL; } +static void +intel_dp_check_link_status(struct intel_dp *intel_dp) +{ + struct intel_encoder *intel_encoder = _to_dig_port(intel_dp)->base; + struct drm_device *dev = intel_dp_to_dev(intel_dp); + u8 link_status[DP_LINK_STATUS_SIZE]; + + WARN_ON(!drm_modeset_is_locked(>mode_config.connection_mutex)); + + if (!intel_dp_get_link_status(intel_dp, link_status)) { + DRM_ERROR("Failed to get link status\n"); + return; + } + + if (!intel_encoder->base.crtc) + return; + + if (!to_intel_crtc(intel_encoder->base.crtc)->active) + return; Why do you change the order of the three if-clauses above? The original order seems to make more sense. (Checking for ->base.crtc and ->active is cheap, whereas accessing AUX to get the link status is time consuming. You don't want to spend that time only to bail out, should one of the other two if-clauses fail.) Best regards, Lukas Actually it is expected to read link status whenever we receive short pulse interrupt irrespective of the panel being enabled or not. So this change is with respect to that rather than any performance based. As a general rule please don't make functional changes like these in a patch that just splits stuff up. Your patch summary sounds like simple refactoring, which this doesn't seem to be. -Daniel Understood, will make the appropriate changes and move that to separate patch. regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Splitting intel_dp_check_link_status
On 1/19/2016 2:35 PM, Daniel Vetter wrote: On Tue, Jan 19, 2016 at 02:29:22PM +0530, Thulasimani, Sivakumar wrote: On 1/19/2016 2:14 PM, Daniel Vetter wrote: On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote: On 1/19/2016 2:35 AM, Lukas Wunner wrote: Hi, On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote: When created originally intel_dp_check_link_status() was supposed to handle only link training for short pulse but has grown into handler for short pulse itself. This patch cleans up this function by splitting it into two halves. First intel_dp_short_pulse() is called, which will be entry point and handle all logic for short pulse handling while intel_dp_check_link_status() will retain its original purpose of only doing link status related work. The link retraining part when EQ is not correct is retained to intel_dp_check_link_status whereas other operations are handled as part of intel_dp_short_pulse. This change is required to avoid performing all DPCD related operations on performing link retraining. v2: Added WARN_ON to intel_dp_check_link_status() Removed a call to intel_dp_get_link_status() (Ander) Tested-by: Nathan D Ciobanu <nathan.d.ciob...@intel.com> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasim...@intel.com> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivast...@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 65 +++-- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 82ee18d..f8d9611 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4279,6 +4279,36 @@ go_again: return -EINVAL; } +static void +intel_dp_check_link_status(struct intel_dp *intel_dp) +{ + struct intel_encoder *intel_encoder = _to_dig_port(intel_dp)->base; + struct drm_device *dev = intel_dp_to_dev(intel_dp); + u8 link_status[DP_LINK_STATUS_SIZE]; + + WARN_ON(!drm_modeset_is_locked(>mode_config.connection_mutex)); + + if (!intel_dp_get_link_status(intel_dp, link_status)) { + DRM_ERROR("Failed to get link status\n"); + return; + } + + if (!intel_encoder->base.crtc) + return; + + if (!to_intel_crtc(intel_encoder->base.crtc)->active) + return; Why do you change the order of the three if-clauses above? The original order seems to make more sense. (Checking for ->base.crtc and ->active is cheap, whereas accessing AUX to get the link status is time consuming. You don't want to spend that time only to bail out, should one of the other two if-clauses fail.) Best regards, Lukas Actually it is expected to read link status whenever we receive short pulse interrupt irrespective of the panel being enabled or not. So this change is with respect to that rather than any performance based. As a general rule please don't make functional changes like these in a patch that just splits stuff up. Your patch summary sounds like simple refactoring, which this doesn't seem to be. -Daniel Understood, will make the appropriate changes and move that to separate patch. btw you don't have to split it since really this is a small change. Changing the subject to something that makes is clearer that it's not just refactoring is also ok, e.g. "reorganize intel_dp_detect" Then explain in the commit message why and what changes, like you do already. -Daniel Sure, that will save some time in redoing ULT+upstreaming :). to give some background, the movement was supposed to be a separate patch but got merged during this cleanup. Will make sure that gets documented and split clearly as required hence forth. regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Splitting intel_dp_check_link_status
On 1/19/2016 2:35 AM, Lukas Wunner wrote: Hi, On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote: When created originally intel_dp_check_link_status() was supposed to handle only link training for short pulse but has grown into handler for short pulse itself. This patch cleans up this function by splitting it into two halves. First intel_dp_short_pulse() is called, which will be entry point and handle all logic for short pulse handling while intel_dp_check_link_status() will retain its original purpose of only doing link status related work. The link retraining part when EQ is not correct is retained to intel_dp_check_link_status whereas other operations are handled as part of intel_dp_short_pulse. This change is required to avoid performing all DPCD related operations on performing link retraining. v2: Added WARN_ON to intel_dp_check_link_status() Removed a call to intel_dp_get_link_status() (Ander) Tested-by: Nathan D CiobanuSigned-off-by: Sivakumar Thulasimani Signed-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/intel_dp.c | 65 +++-- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 82ee18d..f8d9611 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4279,6 +4279,36 @@ go_again: return -EINVAL; } +static void +intel_dp_check_link_status(struct intel_dp *intel_dp) +{ + struct intel_encoder *intel_encoder = _to_dig_port(intel_dp)->base; + struct drm_device *dev = intel_dp_to_dev(intel_dp); + u8 link_status[DP_LINK_STATUS_SIZE]; + + WARN_ON(!drm_modeset_is_locked(>mode_config.connection_mutex)); + + if (!intel_dp_get_link_status(intel_dp, link_status)) { + DRM_ERROR("Failed to get link status\n"); + return; + } + + if (!intel_encoder->base.crtc) + return; + + if (!to_intel_crtc(intel_encoder->base.crtc)->active) + return; Why do you change the order of the three if-clauses above? The original order seems to make more sense. (Checking for ->base.crtc and ->active is cheap, whereas accessing AUX to get the link status is time consuming. You don't want to spend that time only to bail out, should one of the other two if-clauses fail.) Best regards, Lukas Actually it is expected to read link status whenever we receive short pulse interrupt irrespective of the panel being enabled or not. So this change is with respect to that rather than any performance based. regards, Sivakumar + + /* if link training is requested we should perform it always */ + if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || + (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { + DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", + intel_encoder->base.name); + intel_dp_start_link_train(intel_dp); + intel_dp_stop_link_train(intel_dp); + } +} + /* * According to DP spec * 5.1.2: @@ -4288,14 +4318,10 @@ go_again: * 4. Check link status on receipt of hot-plug interrupt */ static void -intel_dp_check_link_status(struct intel_dp *intel_dp) +intel_dp_short_pulse(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); - struct intel_encoder *intel_encoder = _to_dig_port(intel_dp)->base; u8 sink_irq_vector; - u8 link_status[DP_LINK_STATUS_SIZE]; - - WARN_ON(!drm_modeset_is_locked(>mode_config.connection_mutex)); /* * Clearing compliance test variables to allow capturing @@ -4305,17 +4331,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) intel_dp->compliance_test_type = 0; intel_dp->compliance_test_data = 0; - if (!intel_encoder->base.crtc) - return; - - if (!to_intel_crtc(intel_encoder->base.crtc)->active) - return; - - /* Try to read receiver status if the link appears to be up */ - if (!intel_dp_get_link_status(intel_dp, link_status)) { - return; - } - /* Now read the DPCD to see if it's actually running */ if (!intel_dp_get_dpcd(intel_dp)) { return; @@ -4335,14 +4350,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); } - /* if link training is requested we should perform it always */ - if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { - DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", - intel_encoder->base.name); -
Re: [Intel-gfx] [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence
On 12/10/2015 8:32 PM, Ville Syrjälä wrote: On Thu, Dec 10, 2015 at 08:09:01PM +0530, Thulasimani, Sivakumar wrote: On 12/10/2015 7:08 PM, Ville Syrjälä wrote: On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote: On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote: On 12/09/2015 09:35 PM, Ville Syrjälä wrote: On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote: On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilson <ch...@chris-wilson.co.uk> wrote: On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote: On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä <ville.syrj...@linux.intel.com> wrote: On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote: During resume, while turning the EDP panel power on, we need not wait blindly for panel_power_cycle_delay. Check if panel power down sequence in progress and then only wait. This improves our resume time significantly. Signed-off-by: Shobhit Kumar <shobhit.ku...@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index f335c92..10ec669 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp) return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0; } +static bool edp_panel_off_seq(struct intel_dp *intel_dp) +{ + struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct drm_i915_private *dev_priv = dev->dev_private; + + lockdep_assert_held(_priv->pps_mutex); + + if (IS_VALLEYVIEW(dev) && + intel_dp->pps_pipe == INVALID_PIPE) + return false; + + return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0; +} This doens't make sense to me. The power down cycle may have completed just before, and so this would claim we don't have to wait for the power_cycle_delay. Not sure I understand your concern correctly. You are right, power down cycle may have completed just before and if it has then we don't need to wait. But in case the power down cycle is in progress as per internal state, then we need to wait for it to complete. This will happen for example in non-suspend disable path and will be handled correctly. In case of actual suspend/resume, this would have successfully completed and will skip the wait as it is not needed before enabling panel power. + static bool edp_have_panel_vdd(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp) port_name(dp_to_dig_port(intel_dp)->port))) return; - wait_panel_power_cycle(intel_dp); + if (edp_panel_off_seq(intel_dp)) + wait_panel_power_cycle(intel_dp); Looking in from the side, I have no idea what this is meant to do. At the very least you need your explanatory paragraph here which would include what exactly you are waiting for at the start of edp_panel_on (and please try and find a better name for edp_panel_off_seq()). I will add a comment. Basically I am not additionally waiting, but converting the wait which was already there to a conditional wait. The edp_panel_off_seq, checks if panel power down sequence is in progress. In that case we need to wait for the panel power cycle delay. If it is not in that sequence, there is no need to wait. I will make an attempt again on the naming in next patch update. As far I remeber you need to wait for power_cycle_delay between power down cycle and power up cycle. You're trying to throw that wait away entirely, unless the function happens get called while the power down Yes you are right and I realize I made a mistake in my patch which is not checking PP_CYCLE_DELAY_ACTIVE bit. cycle is still in progress. We should already optimize away redundant waits by tracking the end of the power down cycle with the jiffies tracking. Actually looking at the code the power_cycle_delay gets counted from the start of the last power down cycle, so supposedly it's always at least as long as the power down cycle, and typically it's quite a bit longer that that. But that doesn't change the fact that you can't just skip it because the power down cycle delay happened to end already. So what we do now is: 1. initiate power down cycle 2. last_power_cycle=jiffies 3. wait for power down (I suppose this actually waits until the power down delay has passed since that's programmes into the PPS). 4. wait for power_cycle_delay from last_power_cycle 5. initiate power up cycle I think with your patch step 4 would always be skipped since the power down cycle has already ended, and then we fail to honor the power cycle delay. Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that check w
Re: [Intel-gfx] [PATCH] drm/i915: Wait for PP cycle delay only if panel is in power off sequence
On 12/10/2015 7:08 PM, Ville Syrjälä wrote: On Thu, Dec 10, 2015 at 03:15:37PM +0200, Ville Syrjälä wrote: On Thu, Dec 10, 2015 at 03:01:02PM +0530, Kumar, Shobhit wrote: On 12/09/2015 09:35 PM, Ville Syrjälä wrote: On Wed, Dec 09, 2015 at 08:59:26PM +0530, Shobhit Kumar wrote: On Wed, Dec 9, 2015 at 8:34 PM, Chris Wilsonwrote: On Wed, Dec 09, 2015 at 08:07:10PM +0530, Shobhit Kumar wrote: On Wed, Dec 9, 2015 at 7:27 PM, Ville Syrjälä wrote: On Wed, Dec 09, 2015 at 06:51:48PM +0530, Shobhit Kumar wrote: During resume, while turning the EDP panel power on, we need not wait blindly for panel_power_cycle_delay. Check if panel power down sequence in progress and then only wait. This improves our resume time significantly. Signed-off-by: Shobhit Kumar --- drivers/gpu/drm/i915/intel_dp.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index f335c92..10ec669 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -617,6 +617,20 @@ static bool edp_have_panel_power(struct intel_dp *intel_dp) return (I915_READ(_pp_stat_reg(intel_dp)) & PP_ON) != 0; } +static bool edp_panel_off_seq(struct intel_dp *intel_dp) +{ + struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct drm_i915_private *dev_priv = dev->dev_private; + + lockdep_assert_held(_priv->pps_mutex); + + if (IS_VALLEYVIEW(dev) && + intel_dp->pps_pipe == INVALID_PIPE) + return false; + + return (I915_READ(_pp_stat_reg(intel_dp)) & PP_SEQUENCE_POWER_DOWN) != 0; +} This doens't make sense to me. The power down cycle may have completed just before, and so this would claim we don't have to wait for the power_cycle_delay. Not sure I understand your concern correctly. You are right, power down cycle may have completed just before and if it has then we don't need to wait. But in case the power down cycle is in progress as per internal state, then we need to wait for it to complete. This will happen for example in non-suspend disable path and will be handled correctly. In case of actual suspend/resume, this would have successfully completed and will skip the wait as it is not needed before enabling panel power. + static bool edp_have_panel_vdd(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -2025,7 +2039,8 @@ static void edp_panel_on(struct intel_dp *intel_dp) port_name(dp_to_dig_port(intel_dp)->port))) return; - wait_panel_power_cycle(intel_dp); + if (edp_panel_off_seq(intel_dp)) + wait_panel_power_cycle(intel_dp); Looking in from the side, I have no idea what this is meant to do. At the very least you need your explanatory paragraph here which would include what exactly you are waiting for at the start of edp_panel_on (and please try and find a better name for edp_panel_off_seq()). I will add a comment. Basically I am not additionally waiting, but converting the wait which was already there to a conditional wait. The edp_panel_off_seq, checks if panel power down sequence is in progress. In that case we need to wait for the panel power cycle delay. If it is not in that sequence, there is no need to wait. I will make an attempt again on the naming in next patch update. As far I remeber you need to wait for power_cycle_delay between power down cycle and power up cycle. You're trying to throw that wait away entirely, unless the function happens get called while the power down Yes you are right and I realize I made a mistake in my patch which is not checking PP_CYCLE_DELAY_ACTIVE bit. cycle is still in progress. We should already optimize away redundant waits by tracking the end of the power down cycle with the jiffies tracking. Actually looking at the code the power_cycle_delay gets counted from the start of the last power down cycle, so supposedly it's always at least as long as the power down cycle, and typically it's quite a bit longer that that. But that doesn't change the fact that you can't just skip it because the power down cycle delay happened to end already. So what we do now is: 1. initiate power down cycle 2. last_power_cycle=jiffies 3. wait for power down (I suppose this actually waits until the power down delay has passed since that's programmes into the PPS). 4. wait for power_cycle_delay from last_power_cycle 5. initiate power up cycle I think with your patch step 4 would always be skipped since the power down cycle has already ended, and then we fail to honor the power cycle delay. Yes, I agree. I missed checking for PP_CYCLE_DELAY_ACTIVE. Adding that check will take care of this scenario I guess ? Nope. The vdd force bit doesn't respect the PPS state machine, so we must do the waits manually instead. And in theory your patch wouldn't do
Re: [Intel-gfx] intel_dp_detect redesign
On 11/25/2015 3:34 PM, Daniel Vetter wrote: On Tue, Nov 24, 2015 at 08:13:06PM +0100, Daniel Vetter wrote: Hi Ander, Dave had a short discussion about reprobing DP (and MST) state on resume. I think this is something we've missed in our dp hpd handling rework thus far. And at least for SST we need to take it into account since it would be a regression. Currently it's done through ->detect callback from drm_helper_hpd_irq_event called from i915_drm_resume. Also irc logs below. Oh and there's an issue for the hdmi hpd changes that have been merged and reverted too: Those will run into the same problem. Plus in addition doing nothing in ->detect will break storm handling (since that falls back to the probe helper poll work). -Daniel Storm handling is done in i915_hotplug_work_func before detection is called so it should work on top of changes planned. our change is inside intel_dp_detect so any flow before this is called should remain intact. the expected flow post the changes will be digport_work_func -> intel_dp_hpd_pulse if (long pulse) handle long pulse () return IRQ_NONE i915_hotplug_work_func -> detect however good to explicitly check for this, following needs to be tested before sending in next patch/merge 1) MST displays verification (Ander's reported on first set of patches) 2) check behavior on sleep - resume (dave) 3) storm handling needs to be handled as well. (i assume this should be fine, but good to check explicitly) (danvet) regards, Sivakumar Thanks, Daniel danvet: so probing on resume, it seems a bit inconsistent, is the kernel driver meant to be doing it? I think since we stopped vt switching we've stopped doing it, which is making mst docking kinda suck mst was after stopping vt-switching I thought but yeah we should reprobe and we do (at least occasionally if it's not broken again) well people are just noticing it more with mst but not for mst iirc mst just restores and hopes I think but when you suspend in the dock, and move the laptop, and resume things don't work unless you xrandr and vice-versa I looked into it for 5 minutes when tedtso complained an ran ;-) well reproving should bring up/tear down any mst hm, xrandr shouldn't be enough to fix it, we need a real hpd to redo the mst stuff I thought ... so I don't think mst is special here we reprobe through probe helpers janesma, Pali, bleh sorry.. yeah, looks like it needs a stdbool.h.. not sure why I didn't hit that compile error.. sorry about that.. all mst stuff is done directly from hpd since it needs to know long vs. short so it misses out if we probe a DP port and the device is gone, MST will get torn down danvet: not so unless someone else has been hacking the driver * xxmitsu (~m...@5-15-26-95.residential.rdsnet.ro) has joined #dri-devel oh, the completely gone case * danvet looks oh maybe we don't handle that properly oh you might be right, I wonder where we should hook that in drm_helper_hpd_irq_event in i915_drm_resume should get this right for non-mst well non-DP maybe, anderco and rtshiva are reworking this but it's not merged yet I'm guessing detect should not if a port was in mst and is now disconnected ok, skeleton is there but not all intel_dp_detect drm_dp_mst_topology_mgr_resume should probably check the entire hierarchy, not just a simple dpcd write and if anything changes, we need to generate the uevent somewhere so might be better to re-run the entire dp_detect pile tricky part is that we need to lie about long vs. short it should be treated like a long hpd if anything changed, short otherwise well we have mgr->cbs->hotplug in the mst manager already so should reuse that hook airlied, I guess just fix up drm_dp_mst_topology_mgr_resume to do rescan the entire hierarchy calling ->hotplug if anything changed and only returning true if the sink isn't mst any more along the lines of what intel_dp_probe_mst does it's not going to be all that simple ;-) at least if you care about stuff like laptopt connected to dock -> screen s/r doesn't sounds like fun, I'll stick on my list of things to be scared off then laptop only connected to dock airlied, done the same well the use case is laptop in dock, suspend, resume with laptop plugged into a monitor but the fun part is that anderco/rtshiva want to rework this and if they do they'll also break sst dp ;-) so I think I have some victims airlied, yeah that's step one I'd prefer to get the fixes in before redesigning the tower but that already has all the complexity on the driver side only thing missing is the code in the mst helper to rescan the entire tree and call ->hot_plug if needed problem is that current dp hpd handling is a mess already it's hard to fix anything in there atm so fixing this properly is needed anyway it's just that I've forgotten about the resume case for plain DP myself ;-) -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57
Re: [Intel-gfx] [v3] drm/i915/skl: While sanitizing cdclock check the SWF18 as well
Reviewed-by: Sivakumar ThulasimaniOn 11/5/2015 3:05 PM, Shobhit Kumar wrote: SWF18 is set if the display has been intialized by the pre-os. It also gives what configuration is enabled on which pipe. In skl_sanitize_cdclk, the DPLL sanity check can pass even if GOP/VBIOS is not loaded as BIOS enables DPLL for integrated audio codec related programming. So fisrt check if SWF18 is set and then follow through with other DPLL and CDCLK verification. If not set then for sure we need to sanitize the cdclock. v2: Update the commit message for clarity (Siva) v3: Correct the mask to check for bits[23:0] instead of only bits[16:0]. Had missed checking for PIPE C altogether. Remaining are reserved (Siva) Cc: Ville Syrjälä Signed-off-by: Shobhit Kumar --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_display.c | 8 2 files changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9ee9481..bd476ff 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5006,6 +5006,9 @@ enum skl_disp_power_wells { #define SWF1(i) (dev_priv->info.display_mmio_offset + 0x71410 + (i) * 4) #define SWF3(i) (dev_priv->info.display_mmio_offset + 0x72414 + (i) * 4) +/* VBIOS flag for display initialized status */ +#define GEN6_SWF18 (dev_priv->info.display_mmio_offset + 0x4F060) + /* Pipe B */ #define _PIPEBDSL (dev_priv->info.display_mmio_offset + 0x71000) #define _PIPEBCONF(dev_priv->info.display_mmio_offset + 0x71008) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 103cacb..81668b0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5761,6 +5761,14 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) uint32_t cdctl = I915_READ(CDCLK_CTL); int freq = dev_priv->skl_boot_cdclk; + /* +* check if the pre-os intialized the display +* There is SWF18 scratchpad register defined which is set by the +* pre-os which can be used by the OS drivers to check the status +*/ + if ((I915_READ(GEN6_SWF18) & 0x00FF) == 0) + goto sanitize; + /* Is PLL enabled and locked ? */ if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) goto sanitize; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/skl: While sanitizing cdclock check the SWF18 as well
On 11/2/2015 11:17 PM, Kumar, Shobhit wrote: On 11/02/2015 10:07 PM, Thulasimani, Sivakumar wrote: On 11/2/2015 6:49 PM, Kumar, Shobhit wrote: On 11/02/2015 06:40 PM, Jani Nikula wrote: On Mon, 02 Nov 2015, Shobhit Kumar <shobhit.ku...@intel.com> wrote: SWF18 is set if the display has been intialized by the pre-os. It also gives what configuration is enabled on which pipe. The DPLL and CDCLK verification checks can fail as the pre-os does initialize the DPLL for Audio codec initialization. So fisrt check if SWF18 is set and then follow through with other DPLL and CDCLK verification. Can we universally trust all bios/gop/bootloader/whatnot to have initialized this? What if it's not set? As per my discussion with gop team, this has been enabled in main stream for quite sometime including VLV, CHT, BDW, SKL+ and is common for GOP/VBIOS across chrome/windows/android. So yes I think we can universally trust as of now. This has been added since IVB timeframe and should be part of VBT spec. but i just encountered an issue in Android Charging OS where there is no modeset and is using the displays enabled by GOP/VBIOS. This patch might break such expectations. Why would this break anything in Android charging UI. Basically this patch only says do cdclock sanitization if display is not enabled by pre-os. In this use case it is already enabled by GOP/VBIOS and the driver any way expects it to be programmed by pre-os in general. So in this scenario, the sanitization logic will not do anything at all because SWF18 will be set and CDCLOCK and DPLL will be properly enabled already and just return false. Fast-modeset should not be broken by this at all. my bad :( should review carefully when sitting late night. Regards Shobhit BR, Jani. Cc: Ville Syrjälä <ville.syrj...@linux.intel.com> Signed-off-by: Shobhit Kumar <shobhit.ku...@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_display.c | 8 2 files changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9ee9481..bd476ff 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5006,6 +5006,9 @@ enum skl_disp_power_wells { #define SWF1(i) (dev_priv->info.display_mmio_offset + 0x71410 + (i) * 4) #define SWF3(i) (dev_priv->info.display_mmio_offset + 0x72414 + (i) * 4) +/* VBIOS flag for display initialized status */ +#define GEN6_SWF18 (dev_priv->info.display_mmio_offset + 0x4F060) + /* Pipe B */ #define _PIPEBDSL (dev_priv->info.display_mmio_offset + 0x71000) #define _PIPEBCONF (dev_priv->info.display_mmio_offset + 0x71008) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 103cacb..0ecb35c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5761,6 +5761,14 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) uint32_t cdctl = I915_READ(CDCLK_CTL); int freq = dev_priv->skl_boot_cdclk; +/* + * check if the pre-os intialized the display + * There is SWF18 scratchpad register defined which is set by the + * pre-os which can be used by the OS drivers to check the status + */ +if ((I915_READ(GEN6_SWF18) & 0x00) == 0) +goto sanitize; + /* Is PLL enabled and locked ? */ if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) goto sanitize; can you share bit more details on when GOP/VBIOS sets DPLL but does not enable display ? (atleast that is what i understood from the commit message) regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/skl: While sanitizing cdclock check the SWF18 as well
On 11/2/2015 6:49 PM, Kumar, Shobhit wrote: On 11/02/2015 06:40 PM, Jani Nikula wrote: On Mon, 02 Nov 2015, Shobhit Kumarwrote: SWF18 is set if the display has been intialized by the pre-os. It also gives what configuration is enabled on which pipe. The DPLL and CDCLK verification checks can fail as the pre-os does initialize the DPLL for Audio codec initialization. So fisrt check if SWF18 is set and then follow through with other DPLL and CDCLK verification. Can we universally trust all bios/gop/bootloader/whatnot to have initialized this? What if it's not set? As per my discussion with gop team, this has been enabled in main stream for quite sometime including VLV, CHT, BDW, SKL+ and is common for GOP/VBIOS across chrome/windows/android. So yes I think we can universally trust as of now. This has been added since IVB timeframe and should be part of VBT spec. but i just encountered an issue in Android Charging OS where there is no modeset and is using the displays enabled by GOP/VBIOS. This patch might break such expectations. Regards Shobhit BR, Jani. Cc: Ville Syrjälä Signed-off-by: Shobhit Kumar --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_display.c | 8 2 files changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9ee9481..bd476ff 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5006,6 +5006,9 @@ enum skl_disp_power_wells { #define SWF1(i)(dev_priv->info.display_mmio_offset + 0x71410 + (i) * 4) #define SWF3(i)(dev_priv->info.display_mmio_offset + 0x72414 + (i) * 4) +/* VBIOS flag for display initialized status */ +#define GEN6_SWF18 (dev_priv->info.display_mmio_offset + 0x4F060) + /* Pipe B */ #define _PIPEBDSL (dev_priv->info.display_mmio_offset + 0x71000) #define _PIPEBCONF (dev_priv->info.display_mmio_offset + 0x71008) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 103cacb..0ecb35c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5761,6 +5761,14 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) uint32_t cdctl = I915_READ(CDCLK_CTL); int freq = dev_priv->skl_boot_cdclk; +/* + * check if the pre-os intialized the display + * There is SWF18 scratchpad register defined which is set by the + * pre-os which can be used by the OS drivers to check the status + */ +if ((I915_READ(GEN6_SWF18) & 0x00) == 0) +goto sanitize; + /* Is PLL enabled and locked ? */ if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK))) goto sanitize; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Add aux plane verification in addFB2
On 11/2/2015 3:20 PM, Vandana Kannan wrote: For render compression, userspace passes aux stride and offset values as an additional entry in the fb structure. This should not be treated as garbage and discarded as data belonging to no plane. This patch introduces a check related to AUX plane to support the scenario of render compression. Suggested-by: Daniel VetterSigned-off-by: Vandana Kannan --- drivers/gpu/drm/drm_crtc.c | 16 +++- drivers/gpu/drm/drm_ioctl.c | 3 +++ include/drm/drm_crtc.h | 3 +++ include/uapi/drm/drm.h | 1 + include/uapi/drm/drm_mode.h | 1 + 5 files changed, 23 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 24c5434..7dbc0f0 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3204,6 +3204,13 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r) } } + if (r->flags & DRM_MODE_FB_AUX_PLANE) { + if (num_planes == 4) how can num_planes be 4 since the max it can get seems to be 3. regards, Sivakumar + return -EINVAL; + + num_planes++; + } + for (i = num_planes; i < 4; i++) { if (r->modifier[i]) { DRM_DEBUG_KMS("non-zero modifier for unused plane %d\n", i); @@ -3242,7 +3249,8 @@ internal_framebuffer_create(struct drm_device *dev, struct drm_framebuffer *fb; int ret; - if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) { + if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS | + DRM_MODE_FB_AUX_PLANE)) { DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags); return ERR_PTR(-EINVAL); } @@ -3264,6 +3272,12 @@ internal_framebuffer_create(struct drm_device *dev, return ERR_PTR(-EINVAL); } + if (r->flags & DRM_MODE_FB_AUX_PLANE && + !dev->mode_config.allow_aux_plane) { + DRM_DEBUG_KMS("driver does not support render compression\n"); + return ERR_PTR(-EINVAL); + } + ret = framebuffer_check(r); if (ret) return ERR_PTR(ret); diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8ce2a0c..ee00782 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -312,6 +312,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ case DRM_CAP_ADDFB2_MODIFIERS: req->value = dev->mode_config.allow_fb_modifiers; break; + case DRM_CAP_RENDER_COMPRESSION: + req->value = dev->mode_config.allow_aux_plane; + break; default: return -EINVAL; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3f0c690..a5a9da2 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1152,6 +1152,9 @@ struct drm_mode_config { /* whether the driver supports fb modifiers */ bool allow_fb_modifiers; + /* whether the driver supports render compression */ + bool allow_aux_plane; + /* cursor size */ uint32_t cursor_width, cursor_height; }; diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3801584..0834bf7 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -631,6 +631,7 @@ struct drm_gem_open { #define DRM_CAP_CURSOR_WIDTH 0x8 #define DRM_CAP_CURSOR_HEIGHT 0x9 #define DRM_CAP_ADDFB2_MODIFIERS 0x10 +#define DRM_CAP_RENDER_COMPRESSION 0x11 /** DRM_IOCTL_GET_CAP ioctl argument type */ struct drm_get_cap { diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 6c11ca4..de59ace 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -354,6 +354,7 @@ struct drm_mode_fb_cmd { #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */ #define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */ +#define DRM_MODE_FB_AUX_PLANE (1<<2) /* for compressed buffer */ struct drm_mode_fb_cmd2 { __u32 fb_id; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
On 10/23/2015 5:37 PM, R, Durgadoss wrote: -Original Message- From: Ander Conselvan De Oliveira [mailto:conselv...@gmail.com] Sent: Wednesday, October 21, 2015 9:08 PM To: R, Durgadoss; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT On Wed, 2015-10-14 at 17:30 +0530, Durgadoss R wrote: To support USB type C alternate DP mode, the display driver needs to know the number of lanes required by the DP panel as well as number of lanes that can be supported by the type-C cable. Sometimes, the type-C cable may limit the bandwidth even if Panel can support more lanes. To address these scenarios, the display driver will start link training with max lanes, and if that fails, the driver falls back to x2 lanes; and repeats this procedure for all bandwidth/lane configurations. * Since link training is done before modeset only the port (and not pipe/planes) and its associated PLLs are enabled. * Once link training is done, the port and its PLLs are disabled; so that the subsequent modeset is not aware of these changes. * On DP hotplug: Directly start link training on the crtc associated with the DP encoder. * On Connected boot scenarios: When booted with an LFP and a DP, typically, BIOS brings up DP. In these cases, we disable the crtc first and then start upfront link training. The crtc is re-enabled as part of a subsequent modeset. * For BXT, ddi->enable/disable for DP only enable/disable audio codec and hence are not included in upfront link training sequence. * As of now, this is tested only on BXT A1 platform, on kernel 4.2-rc2. Signed-off-by: Durgadoss R--- drivers/gpu/drm/i915/intel_ddi.c | 152 +++ drivers/gpu/drm/i915/intel_dp.c | 41 ++- drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 194 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 8e4ea36..b3a9bff 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3209,6 +3209,158 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port) return connector; } +bool intel_ddi_upfront_link_train(struct drm_device *dev, + struct intel_dp *intel_dp, struct intel_crtc *crtc) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); + struct intel_connector *connector = intel_dp->attached_connector; + struct intel_encoder *tmp_encoder, *encoder = connector->encoder; + struct intel_shared_dpll *pll; + struct intel_crtc *tmp_crtc; + struct drm_crtc *tmp_drm_crtc; + uint8_t tmp_lane_count, tmp_link_bw; + bool ret, found, valid_crtc = false; + + /* For now, we have only SKL and BXT supporting type-C */ + if (!IS_BROXTON(dev) || !IS_SKYLAKE(dev)) + return true; + + if (!connector || !encoder) { + DRM_DEBUG_KMS("dp connector/encoder is NULL\n"); + return false; + } + + /* If we already have a crtc, start link training directly */ + if (crtc) { + valid_crtc = true; + goto start_link_train; + } + + /* Find an unused crtc and use it for link training */ + for_each_intel_crtc(dev, crtc) { + if (intel_crtc_active(>base)) + continue; + + /* Make sure the new crtc will work with the encoder */ + if (drm_encoder_crtc_ok(>base, >base)) { + found = true; + + /* Save the existing values */ + tmp_encoder = connector->new_encoder; + tmp_crtc = encoder->new_crtc; + tmp_drm_crtc = encoder->base.crtc; In which case are these different than NULL? I thought at this point there hasn't been a modeset in the hotplug case and you disable the crtc on the connected on boot case. This will also need to be rebased on atomic. As far as I tested these, they are NULL in both Hotplug and connected boot Cases. There was one issue during suspend/resume where it was not NULL. But later we figured out, we always have a valid crtc during resume and hence Should not take this path. So, yes, with all our testing so far, NULL works fine here. Agreed, this need to be rebased on atomic. Will do this in next version. + + connector->new_encoder = encoder; + encoder->new_crtc = crtc; + encoder->base.crtc = >base; + + break; + } + } I think it would be a good idea to split the search for an unused crtc to a separate function. Also, there's similar code in intel_get_load_detect_pipe(), it would be nice if that could use the same
Re: [Intel-gfx] [PATCH 01/22] drm/i915: Don't pass *DP around to link training functions
Thanks for making the changes suggested :) Reviewed-by: Sivakumar ThulasimaniOn 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: It just makes the code more confusing, so just reference intel_dp_>DP directly. Note that this also fix a bug where the value of intel_dp->DP could be different than the last value written to the hw, due to an early return that would skip the 'intel_dp->DP = DP' line. v2: Don't preserve old DP value on failure. (Sivakumar) - Don't call drm_dp_clock_recovery_ok() twice. (Sivakumar) - Keep return type of clock recovery and channel equalization functions as void. (Ander) v3: Remove DP parameter from intel_dp_set_signal_levels(). (Sivakumar) Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_dp.c | 47 ++--- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 8287df4..648300e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3552,7 +3552,7 @@ gen7_edp_signal_levels(uint8_t train_set) /* Properly updates "DP" with the correct signal levels. */ static void -intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP) +intel_dp_set_signal_levels(struct intel_dp *intel_dp) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); enum port port = intel_dig_port->port; @@ -3591,12 +3591,11 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP) (train_set & DP_TRAIN_PRE_EMPHASIS_MASK) >> DP_TRAIN_PRE_EMPHASIS_SHIFT); - *DP = (*DP & ~mask) | signal_levels; + intel_dp->DP = (intel_dp->DP & ~mask) | signal_levels; } static bool intel_dp_set_link_train(struct intel_dp *intel_dp, - uint32_t *DP, uint8_t dp_train_pat) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3605,9 +3604,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, uint8_t buf[sizeof(intel_dp->train_set) + 1]; int ret, len; - _intel_dp_set_link_train(intel_dp, DP, dp_train_pat); + _intel_dp_set_link_train(intel_dp, _dp->DP, dp_train_pat); - I915_WRITE(intel_dp->output_reg, *DP); + I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); buf[0] = dp_train_pat; @@ -3628,17 +3627,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, } static bool -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, +intel_dp_reset_link_train(struct intel_dp *intel_dp, uint8_t dp_train_pat) { if (!intel_dp->train_set_valid) memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); - intel_dp_set_signal_levels(intel_dp, DP); - return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); + intel_dp_set_signal_levels(intel_dp); + return intel_dp_set_link_train(intel_dp, dp_train_pat); } static bool -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, +intel_dp_update_link_train(struct intel_dp *intel_dp, const uint8_t link_status[DP_LINK_STATUS_SIZE]) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3647,9 +3646,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, int ret; intel_get_adjust_train(intel_dp, link_status); - intel_dp_set_signal_levels(intel_dp, DP); + intel_dp_set_signal_levels(intel_dp); - I915_WRITE(intel_dp->output_reg, *DP); + I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); ret = drm_dp_dpcd_write(_dp->aux, DP_TRAINING_LANE0_SET, @@ -3698,7 +3697,6 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) int i; uint8_t voltage; int voltage_tries, loop_tries; - uint32_t DP = intel_dp->DP; uint8_t link_config[2]; uint8_t link_bw, rate_select; @@ -3722,10 +3720,10 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) link_config[1] = DP_SET_ANSI_8B10B; drm_dp_dpcd_write(_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2); - DP |= DP_PORT_EN; + intel_dp->DP |= DP_PORT_EN; /* clock recovery */ - if (!intel_dp_reset_link_train(intel_dp, , + if (!intel_dp_reset_link_train(intel_dp, DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABLE)) { DRM_ERROR("failed to enable link training\n"); @@ -3757,7 +3755,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) DRM_DEBUG_KMS("clock recovery not ok, reset"); /* clear the flag as
Re: [Intel-gfx] [PATCH 05/22] drm/i915: Move generic link training code to a separate file
Reviewed-by: Sivakumar ThulasimaniOn 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: No functional changes, just moving code around. v2: Rebase Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/intel_dp.c | 321 + drivers/gpu/drm/i915/intel_dp_link_training.c | 327 ++ drivers/gpu/drm/i915/intel_drv.h | 16 ++ 4 files changed, 353 insertions(+), 312 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_dp_link_training.c diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 44d290a..0851de07 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -77,6 +77,7 @@ i915-y += dvo_ch7017.o \ dvo_tfp410.o \ intel_crt.o \ intel_ddi.o \ + intel_dp_link_training.o \ intel_dp_mst.o \ intel_dp.o \ intel_dsi.o \ diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 4ffb2e3..4b88823 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1189,7 +1189,7 @@ intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates) return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; } -static bool intel_dp_source_supports_hbr2(struct drm_device *dev) +bool intel_dp_source_supports_hbr2(struct drm_device *dev) { /* WaDisableHBR2:skl */ if (IS_SKL_REVID(dev, 0, SKL_REVID_B0)) @@ -1365,8 +1365,8 @@ int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) return rate_to_index(rate, intel_dp->sink_rates); } -static void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock, - uint8_t *link_bw, uint8_t *rate_select) +void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock, + uint8_t *link_bw, uint8_t *rate_select) { if (intel_dp->num_sink_rates) { *link_bw = 0; @@ -3046,7 +3046,7 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset, * Fetch AUX CH registers 0x202 - 0x207 which contain * link status information */ -static bool +bool intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]) { return intel_dp_dpcd_read_wake(_dp->aux, @@ -3056,7 +3056,7 @@ intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_ } /* These are source-specific values. */ -static uint8_t +uint8_t intel_dp_voltage_max(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -3079,7 +3079,7 @@ intel_dp_voltage_max(struct intel_dp *intel_dp) return DP_TRAIN_VOLTAGE_SWING_LEVEL_2; } -static uint8_t +uint8_t intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -3421,38 +3421,6 @@ static uint32_t chv_signal_levels(struct intel_dp *intel_dp) return 0; } -static void -intel_get_adjust_train(struct intel_dp *intel_dp, - const uint8_t link_status[DP_LINK_STATUS_SIZE]) -{ - uint8_t v = 0; - uint8_t p = 0; - int lane; - uint8_t voltage_max; - uint8_t preemph_max; - - for (lane = 0; lane < intel_dp->lane_count; lane++) { - uint8_t this_v = drm_dp_get_adjust_request_voltage(link_status, lane); - uint8_t this_p = drm_dp_get_adjust_request_pre_emphasis(link_status, lane); - - if (this_v > v) - v = this_v; - if (this_p > p) - p = this_p; - } - - voltage_max = intel_dp_voltage_max(intel_dp); - if (v >= voltage_max) - v = voltage_max | DP_TRAIN_MAX_SWING_REACHED; - - preemph_max = intel_dp_pre_emphasis_max(intel_dp, v); - if (p >= preemph_max) - p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED; - - for (lane = 0; lane < 4; lane++) - intel_dp->train_set[lane] = v | p; -} - static uint32_t gen4_signal_levels(uint8_t train_set) { @@ -3550,7 +3518,7 @@ gen7_edp_signal_levels(uint8_t train_set) } } -static void +void intel_dp_set_signal_levels(struct intel_dp *intel_dp) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3597,7 +3565,7 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp) POSTING_READ(intel_dp->output_reg); } -static void +void intel_dp_program_link_training_pattern(struct intel_dp *intel_dp, uint8_t dp_train_pat) { @@ -3611,56 +3579,7 @@ intel_dp_program_link_training_pattern(struct intel_dp *intel_dp, POSTING_READ(intel_dp->output_reg); }
Re: [Intel-gfx] [PATCH 07/22] drm/i915: Make intel_dp_source_supports_hbr2() take an intel_dp pointer
Reviewed-by: Sivakumar ThulasimaniOn 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: The function name implies it should get intel_dp, and it mostly used where there is an intel_dp in the context. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_dp.c | 19 +++ drivers/gpu/drm/i915/intel_dp_link_training.c | 4 +--- drivers/gpu/drm/i915/intel_drv.h | 2 +- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 5b04ade..5344de4 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1189,8 +1189,11 @@ intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates) return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; } -bool intel_dp_source_supports_hbr2(struct drm_device *dev) +bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp) { + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = dig_port->base.base.dev; + /* WaDisableHBR2:skl */ if (IS_SKL_REVID(dev, 0, SKL_REVID_B0)) return false; @@ -1203,8 +1206,10 @@ bool intel_dp_source_supports_hbr2(struct drm_device *dev) } static int -intel_dp_source_rates(struct drm_device *dev, const int **source_rates) +intel_dp_source_rates(struct intel_dp *intel_dp, const int **source_rates) { + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = dig_port->base.base.dev; int size; if (IS_BROXTON(dev)) { @@ -1219,7 +1224,7 @@ intel_dp_source_rates(struct drm_device *dev, const int **source_rates) } /* This depends on the fact that 5.4 is last value in the array */ - if (!intel_dp_source_supports_hbr2(dev)) + if (!intel_dp_source_supports_hbr2(intel_dp)) size--; return size; @@ -1284,12 +1289,11 @@ static int intersect_rates(const int *source_rates, int source_len, static int intel_dp_common_rates(struct intel_dp *intel_dp, int *common_rates) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); const int *source_rates, *sink_rates; int source_len, sink_len; sink_len = intel_dp_sink_rates(intel_dp, _rates); - source_len = intel_dp_source_rates(dev, _rates); + source_len = intel_dp_source_rates(intel_dp, _rates); return intersect_rates(source_rates, source_len, sink_rates, sink_len, @@ -1314,7 +1318,6 @@ static void snprintf_int_array(char *str, size_t len, static void intel_dp_print_rates(struct intel_dp *intel_dp) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); const int *source_rates, *sink_rates; int source_len, sink_len, common_len; int common_rates[DP_MAX_SUPPORTED_RATES]; @@ -1323,7 +1326,7 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp) if ((drm_debug & DRM_UT_KMS) == 0) return; - source_len = intel_dp_source_rates(dev, _rates); + source_len = intel_dp_source_rates(intel_dp, _rates); snprintf_int_array(str, sizeof(str), source_rates, source_len); DRM_DEBUG_KMS("source rates: %s\n", str); @@ -3711,7 +3714,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) } DRM_DEBUG_KMS("Display Port TPS3 support: source %s, sink %s\n", - yesno(intel_dp_source_supports_hbr2(dev)), + yesno(intel_dp_source_supports_hbr2(intel_dp)), yesno(drm_dp_tps3_supported(intel_dp->dpcd))); /* Intermediate frequency support */ diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index bb036d5..793 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -218,8 +218,6 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) static void intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) { - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); - struct drm_device *dev = dig_port->base.base.dev; bool channel_eq = false; int tries, cr_tries; uint32_t training_pattern = DP_TRAINING_PATTERN_2; @@ -233,7 +231,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) * Due to WaDisableHBR2 SKL < B0 is the only exception where TPS3 is * supported but still not enabled. */ - if (intel_dp_source_supports_hbr2(dev) && + if (intel_dp_source_supports_hbr2(intel_dp) && drm_dp_tps3_supported(intel_dp->dpcd)) training_pattern = DP_TRAINING_PATTERN_3; else if (intel_dp->link_rate == 54) diff --git
Re: [Intel-gfx] [PATCH 16/22] drm/i915: Introduce struct intel_dp_signal_levels
This is a good simplification but for a wrong implementation :). we know that our current implementation is wrong and needs to be done the opposite way to make our code compliant with spec. i assume that it will then be a question of when this will be removed rather than if. In such a scenario is this and following patches/optimizations needed "at present" ? regards, Sivakumar On 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: In order to clarify which platforms support which combination of voltage swing and pre emphasis level, introduce struct intel_dp_signal_levels. With the new struct, the if ladder to determine the values used is put in one place, intel_dp_init_signal_levels(). This also wires gens 4, 5 and non-eDP ports on gen 6 and 7 to use the new struct. v2: Rebase Signed-off-by: Ander Conselvan de Oliveira--- drivers/gpu/drm/i915/intel_dp.c | 2 + drivers/gpu/drm/i915/intel_dp_signal_levels.c | 103 ++ drivers/gpu/drm/i915/intel_drv.h | 11 +++ 3 files changed, 101 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 1236791..e640b59 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5220,6 +5220,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, if (HAS_DDI(dev)) intel_dp->prepare_link_retrain = intel_ddi_prepare_link_retrain; + intel_dp_init_signal_levels(intel_dp); + /* Preserve the current hw state. */ intel_dp->DP = I915_READ(intel_dp->output_reg); intel_dp->attached_connector = intel_connector; diff --git a/drivers/gpu/drm/i915/intel_dp_signal_levels.c b/drivers/gpu/drm/i915/intel_dp_signal_levels.c index e516dd2..365bdc4 100644 --- a/drivers/gpu/drm/i915/intel_dp_signal_levels.c +++ b/drivers/gpu/drm/i915/intel_dp_signal_levels.c @@ -24,8 +24,8 @@ #include "intel_drv.h" /* These are source-specific values. */ -uint8_t -intel_dp_voltage_max(struct intel_dp *intel_dp) +static uint8_t +_dp_voltage_max(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); struct drm_i915_private *dev_priv = dev->dev_private; @@ -47,8 +47,8 @@ intel_dp_voltage_max(struct intel_dp *intel_dp) return DP_TRAIN_VOLTAGE_SWING_LEVEL_2; } -uint8_t -intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing) +static uint8_t +_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing) { struct drm_device *dev = intel_dp_to_dev(intel_dp); enum port port = dp_to_dig_port(intel_dp)->port; @@ -389,10 +389,10 @@ static uint32_t chv_signal_levels(struct intel_dp *intel_dp) return 0; } -static uint32_t -gen4_signal_levels(uint8_t train_set) +static void +gen4_set_signal_levels(struct intel_dp *intel_dp, uint8_t train_set) { - uint32_tsignal_levels = 0; + uint32_t signal_levels = 0; switch (train_set & DP_TRAIN_VOLTAGE_SWING_MASK) { case DP_TRAIN_VOLTAGE_SWING_LEVEL_0: @@ -424,9 +424,38 @@ gen4_signal_levels(uint8_t train_set) signal_levels |= DP_PRE_EMPHASIS_9_5; break; } - return signal_levels; + + DRM_DEBUG_KMS("Using signal levels %08x\n", signal_levels); + + intel_dp->DP &= ~(DP_VOLTAGE_MASK | DP_PRE_EMPHASIS_MASK); + intel_dp->DP |= signal_levels; } +static const struct signal_levels gen4_signal_levels = { + .max_voltage = DP_TRAIN_VOLTAGE_SWING_LEVEL_2, + .max_pre_emph = { + DP_TRAIN_PRE_EMPH_LEVEL_2, + DP_TRAIN_PRE_EMPH_LEVEL_2, + DP_TRAIN_PRE_EMPH_LEVEL_1, + DP_TRAIN_PRE_EMPH_LEVEL_0, + }, + + .set = gen4_set_signal_levels, +}; + +/* Not for eDP */ +static const struct signal_levels snb_signal_levels = { + .max_voltage = DP_TRAIN_VOLTAGE_SWING_LEVEL_3, + .max_pre_emph = { + DP_TRAIN_PRE_EMPH_LEVEL_2, + DP_TRAIN_PRE_EMPH_LEVEL_2, + DP_TRAIN_PRE_EMPH_LEVEL_1, + DP_TRAIN_PRE_EMPH_LEVEL_0, + }, + + .set = gen4_set_signal_levels, +}; + /* Gen6's DP voltage swing and pre-emphasis control */ static uint32_t gen6_edp_signal_levels(uint8_t train_set) @@ -486,13 +515,12 @@ gen7_edp_signal_levels(uint8_t train_set) } } -void -intel_dp_set_signal_levels(struct intel_dp *intel_dp) +static void +_update_signal_levels(struct intel_dp *intel_dp) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); enum port port = intel_dig_port->port; struct drm_device *dev = intel_dig_port->base.base.dev; - struct drm_i915_private *dev_priv = to_i915(dev); uint32_t signal_levels, mask = 0; uint8_t train_set = intel_dp->train_set[0]; @@ -514,21 +542,66 @@
Re: [Intel-gfx] [PATCH 02/22] drm/i915: Split write of pattern to DP reg from intel_dp_set_link_train
Reviewed-by: Sivakumar ThulasimaniOn 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: Split the register write with the new link training pattern out of intel_dp_set_link_train(), so that the i915 specific code is in a separate function. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_dp.c | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 648300e..0958cab 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3594,20 +3594,28 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp) intel_dp->DP = (intel_dp->DP & ~mask) | signal_levels; } -static bool -intel_dp_set_link_train(struct intel_dp *intel_dp, - uint8_t dp_train_pat) +static void +intel_dp_program_link_training_pattern(struct intel_dp *intel_dp, + uint8_t dp_train_pat) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev); - uint8_t buf[sizeof(intel_dp->train_set) + 1]; - int ret, len; _intel_dp_set_link_train(intel_dp, _dp->DP, dp_train_pat); I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); +} + +static bool +intel_dp_set_link_train(struct intel_dp *intel_dp, + uint8_t dp_train_pat) +{ + uint8_t buf[sizeof(intel_dp->train_set) + 1]; + int ret, len; + + intel_dp_program_link_training_pattern(intel_dp, dp_train_pat); buf[0] = dp_train_pat; if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) == ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/22] drm/i915: Move register write into intel_dp_set_signal_levels()
Reviewed-by: Sivakumar ThulasimaniOn 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: Move register write from intel_dp_update_link_train() into intel_dp_set_signal_levels(). This creates a better split between the i915 specific code and the generic link training part. Note that this causes an extra register write in intel_dp_reset_link_train(), since both intel_dp_set_signal_levels() and intel_dp_set_link_train() write to the DP register. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_dp.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 11f2385..4ffb2e3 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3550,13 +3550,13 @@ gen7_edp_signal_levels(uint8_t train_set) } } -/* Properly updates "DP" with the correct signal levels. */ static void intel_dp_set_signal_levels(struct intel_dp *intel_dp) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); enum port port = intel_dig_port->port; struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = to_i915(dev); uint32_t signal_levels, mask = 0; uint8_t train_set = intel_dp->train_set[0]; @@ -3592,6 +3592,9 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp) DP_TRAIN_PRE_EMPHASIS_SHIFT); intel_dp->DP = (intel_dp->DP & ~mask) | signal_levels; + + I915_WRITE(intel_dp->output_reg, intel_dp->DP); + POSTING_READ(intel_dp->output_reg); } static void @@ -3647,16 +3650,10 @@ intel_dp_reset_link_train(struct intel_dp *intel_dp, static bool intel_dp_update_link_train(struct intel_dp *intel_dp) { - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); - struct drm_i915_private *dev_priv = - to_i915(intel_dig_port->base.base.dev); int ret; intel_dp_set_signal_levels(intel_dp); - I915_WRITE(intel_dp->output_reg, intel_dp->DP); - POSTING_READ(intel_dp->output_reg); - ret = drm_dp_dpcd_write(_dp->aux, DP_TRAINING_LANE0_SET, intel_dp->train_set, intel_dp->lane_count); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/22] drm/i915: Add function for getting the current link training voltage
On 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: Add a function for retrieving the current voltage swing being used for link training. Using that function in the clock recovery code makes it a bit more readable. Signed-off-by: Ander Conselvan de Oliveira--- drivers/gpu/drm/i915/intel_dp_link_training.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index 9fdcc77..e5b9410 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -155,6 +155,12 @@ max_voltage_reached_on_all_lanes(struct intel_dp *intel_dp) return true; } +static uint8_t +intel_dp_get_train_voltage(struct intel_dp *intel_dp) +{ + return intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK; +} + /* Enable corresponding port and start training pattern 1 */ static void intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) @@ -214,7 +220,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) } /* Check to see if we've tried the same voltage 5 times */ - if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) { + if (intel_dp_get_train_voltage(intel_dp) == voltage) { i understand this is related to taste/preference but wouldn't a macro be enough here ? something like #define IS_MAX_SWING(train_set)(train_set & DP_TRAIN_VOLTAGE_SWING_MASK) regards, Sivakumar ++voltage_tries; if (voltage_tries == 5) { DRM_ERROR("too many voltage retries, give up\n"); @@ -222,7 +228,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) } } else voltage_tries = 0; - voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK; + voltage = intel_dp_get_train_voltage(intel_dp); /* Update training set as requested by target */ intel_get_adjust_train(intel_dp, link_status); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/22] drm/i915 Call get_adjust_train() from clock recovery and channel eq
Reviewed-by: Sivakumar ThulasimaniOn 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: Move the call to intel_dp_get_adjust_train() out of intel_dp_update_link_train() and call it instead from the clock recovery and channel equalization features. A follow up patch will remove the DP register write from that function, so that it handles only the DPCD write. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_dp.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 0958cab..11f2385 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3645,15 +3645,13 @@ intel_dp_reset_link_train(struct intel_dp *intel_dp, } static bool -intel_dp_update_link_train(struct intel_dp *intel_dp, - const uint8_t link_status[DP_LINK_STATUS_SIZE]) +intel_dp_update_link_train(struct intel_dp *intel_dp) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev); int ret; - intel_get_adjust_train(intel_dp, link_status); intel_dp_set_signal_levels(intel_dp); I915_WRITE(intel_dp->output_reg, intel_dp->DP); @@ -3801,7 +3799,8 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK; /* Update training set as requested by target */ - if (!intel_dp_update_link_train(intel_dp, link_status)) { + intel_get_adjust_train(intel_dp, link_status); + if (!intel_dp_update_link_train(intel_dp)) { DRM_ERROR("failed to update link training\n"); break; } @@ -3888,7 +3887,8 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) } /* Update training set as requested by target */ - if (!intel_dp_update_link_train(intel_dp, link_status)) { + intel_get_adjust_train(intel_dp, link_status); + if (!intel_dp_update_link_train(intel_dp)) { DRM_ERROR("failed to update link training\n"); break; } ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/22] drm/i915: Create intel_dp->prepare_link_retrain() hook
Reviewed-by: Sivakumar ThulasimaniOn 10/23/2015 3:31 PM, Ander Conselvan de Oliveira wrote: In order to prepare for a link training with DDI, the state machine would call intel_ddi_prepare_link_retrain(). To remove the dependency to the hardware information, replace that direct call with a callback. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_ddi.c | 8 drivers/gpu/drm/i915/intel_dp.c | 3 +++ drivers/gpu/drm/i915/intel_dp_link_training.c | 6 ++ drivers/gpu/drm/i915/intel_drv.h | 6 +- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index a163741..80843d9 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2971,11 +2971,11 @@ void intel_ddi_pll_init(struct drm_device *dev) } } -void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder) +void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp) { - struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); - struct intel_dp *intel_dp = _dig_port->dp; - struct drm_i915_private *dev_priv = encoder->dev->dev_private; + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_i915_private *dev_priv = + to_i915(intel_dig_port->base.base.dev); enum port port = intel_dig_port->port; uint32_t val; bool wait = false; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 4b88823..5b04ade 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5731,6 +5731,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, else intel_dp->get_aux_send_ctl = i9xx_get_aux_send_ctl; + if (HAS_DDI(dev)) + intel_dp->prepare_link_retrain = intel_ddi_prepare_link_retrain; + /* Preserve the current hw state. */ intel_dp->DP = I915_READ(intel_dp->output_reg); intel_dp->attached_connector = intel_connector; diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index 53f9c14..bb036d5 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -108,16 +108,14 @@ intel_dp_update_link_train(struct intel_dp *intel_dp) static void intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) { - struct drm_encoder *encoder = _to_dig_port(intel_dp)->base.base; - struct drm_device *dev = encoder->dev; int i; uint8_t voltage; int voltage_tries, loop_tries; uint8_t link_config[2]; uint8_t link_bw, rate_select; - if (HAS_DDI(dev)) - intel_ddi_prepare_link_retrain(encoder); + if (intel_dp->prepare_link_retrain) + intel_dp->prepare_link_retrain(intel_dp); intel_dp_compute_rate(intel_dp, intel_dp->link_rate, _bw, _select); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 25ac5d2..1c2a8ed 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -784,6 +784,10 @@ struct intel_dp { bool has_aux_irq, int send_bytes, uint32_t aux_clock_divider); + + /* This is called before a link training is starterd */ + void (*prepare_link_retrain)(struct intel_dp *intel_dp); + bool train_set_valid; /* Displayport compliance testing */ @@ -988,7 +992,7 @@ void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc); bool intel_ddi_pll_select(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); -void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder); +void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp); bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector); void intel_ddi_fdi_disable(struct drm_crtc *crtc); void intel_ddi_get_config(struct intel_encoder *encoder, ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Don't pass *DP around to link training functions
On 10/21/2015 7:22 PM, Ander Conselvan de Oliveira wrote: It just makes the code more confusing, so just reference intel_dp->DP directly. Note that this also fix a bug where the value of intel_dp->DP could be different than the last value written to the hw, due to an early return that would skip the 'intel_dp->DP = DP' line. v2: Don't preserve old DP value on failure. (Sivakumar) - Don't call drm_dp_clock_recovery_ok() twice. (Sivakumar) - Keep return type of clock recovery and channel equalization functions as void. (Ander) Signed-off-by: Ander Conselvan de Oliveira--- drivers/gpu/drm/i915/intel_dp.c | 43 + 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 8287df4..f310dab 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3596,7 +3596,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP) static bool intel_dp_set_link_train(struct intel_dp *intel_dp, - uint32_t *DP, uint8_t dp_train_pat) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3605,9 +3604,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, uint8_t buf[sizeof(intel_dp->train_set) + 1]; int ret, len; - _intel_dp_set_link_train(intel_dp, DP, dp_train_pat); + _intel_dp_set_link_train(intel_dp, _dp->DP, dp_train_pat); - I915_WRITE(intel_dp->output_reg, *DP); + I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); buf[0] = dp_train_pat; @@ -3628,17 +3627,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, } static bool -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, +intel_dp_reset_link_train(struct intel_dp *intel_dp, uint8_t dp_train_pat) { if (!intel_dp->train_set_valid) memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); - intel_dp_set_signal_levels(intel_dp, DP); - return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); + intel_dp_set_signal_levels(intel_dp, _dp->DP); can this be removed as well ? we are already passing intel_dp so same as other places. + return intel_dp_set_link_train(intel_dp, dp_train_pat); } static bool -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, +intel_dp_update_link_train(struct intel_dp *intel_dp, const uint8_t link_status[DP_LINK_STATUS_SIZE]) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3647,9 +3646,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, int ret; intel_get_adjust_train(intel_dp, link_status); - intel_dp_set_signal_levels(intel_dp, DP); + intel_dp_set_signal_levels(intel_dp, _dp->DP); - I915_WRITE(intel_dp->output_reg, *DP); + I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); ret = drm_dp_dpcd_write(_dp->aux, DP_TRAINING_LANE0_SET, @@ -3698,7 +3697,6 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) int i; uint8_t voltage; int voltage_tries, loop_tries; - uint32_t DP = intel_dp->DP; uint8_t link_config[2]; uint8_t link_bw, rate_select; @@ -3722,10 +3720,10 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) link_config[1] = DP_SET_ANSI_8B10B; drm_dp_dpcd_write(_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2); - DP |= DP_PORT_EN; + intel_dp->DP |= DP_PORT_EN; /* clock recovery */ - if (!intel_dp_reset_link_train(intel_dp, , + if (!intel_dp_reset_link_train(intel_dp, DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABLE)) { DRM_ERROR("failed to enable link training\n"); @@ -3757,7 +3755,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) DRM_DEBUG_KMS("clock recovery not ok, reset"); /* clear the flag as we are not reusing train set */ intel_dp->train_set_valid = false; - if (!intel_dp_reset_link_train(intel_dp, , + if (!intel_dp_reset_link_train(intel_dp, DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABLE)) { DRM_ERROR("failed to enable link training\n"); @@ -3776,7 +3774,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) DRM_ERROR("too many full retries, give up\n"); break; } -
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Fix random aux transactions failures.
On 10/21/2015 12:53 PM, Daniel Vetter wrote: On Wed, Oct 21, 2015 at 09:18:06AM +0200, Daniel Vetter wrote: On Wed, Oct 21, 2015 at 10:28:53AM -0700, Rodrigo Vivi wrote: Mainly aux communications on sink_crc were failing a lot randomly on recent platforms. The first solution was to try to use intel_dp_dpcd_read_wake, but then it was suggested to move retries to drm level. Since drm level was already taking care of retries and didn't want to through random retries on that level the second solution was to put the retries at aux_transfer layer what was nacked. So I realized we had so many retries in different places and started to organize that a bit. During this organization I noticed that we weren't handing at all the case were the message size was zeroed. And this was exactly the case that was affecting sink_crc. Also we weren't respect BSPec who says this size message = 0 or > 20 are forbidden. It is a fact that we still have no clue why we are getting this forbidden value there. But anyway we need to handle that for now so we return -EBUSY and drm level takes care of the retries that are already in place. Cc: Jani NikulaCc: Daniel Vetter Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/intel_dp.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index aa3d8f6..80850d6 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -911,6 +911,17 @@ done: /* Unload any bytes sent back from the other side */ recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >> DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT); + + /* +* By BSpec: "Message sizes of 0 or >20 are not allowed." +* We have no idea of what happened so we return -EBUSY so +* drm layer takes care for the necessary retries. +*/ + if (recv_bytes == 0 || recv_bytes > 20) { + ret = -EBUSY; + goto out; + } Hm, this should be caught be the dp aux helper library. Both callers for ->transfer should check for this and reject with -EINVAL (since such a transaction is simply not allowed by dp aux). In the case of drm_dp_i2c_do_msg maybe even with a WARN_ON since the i2c logic should split things up correctly. Meh, totally misread what's going on here, this is from the hardware. How does this even happen? Do you get some kind of garbage value? Should we maybe clear this register field first? It certainly would explain a lot of our random dp aux retry fun ... -Daniel we are already checking for read size in intel_dp_aux_transfer if (WARN_ON(rxsize > 20)) return -E2BIG; and again inside intel_dp_aux_ch 843 /* Only 5 data registers! */ 844 if (WARN_ON(send_bytes > 20 || recv_size > 20)) { 845 ret = -E2BIG; 846 goto out; 847 } Also isn't it possible that a simple write command will have 0 for receive size ? can you share bit more details on what scenario this patch is helping ? regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use dpcd read wake for sink crc calls.
On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: On Mon, 2015-08-24 at 19:54 +, Zanoni, Paulo R wrote: Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: Let's use a native read with retry as suggested per spec to fix Sink CRC on SKL when PSR is enabled. With PSR enabled panel is probably taking more time to wake and dpcd read is faling. Does this commit actually fix any known problem with Sink CRC? Or is it just a try? It would be nice to have this clarified in the commit message. It was just a try but that made sink crc working on my SKL when PSR is enabled. nothing much to add... SKL has new register AUX_MUTEX which should be used when accessing dpcd on edp. just searched the nightly code and could not find it. it might be the reason for random dpcd failures reported in the other thread. regards, Sivakumar Anyway, it looks correct, so: Reviewed-by: Paulo Zanoniv2: Fix my email domain on commit message. Thanks Rafael. Cc: Rafael Antognolli Cc: Sonika Jindal Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/intel_dp.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index d32ce48..34f5e33 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp) u8 buf; int ret = 0; - if (drm_dp_dpcd_readb(_dp->aux, DP_TEST_SINK, ) < 0) { + if (intel_dp_dpcd_read_wake(_dp->aux, DP_TEST_SINK, + , 1) < 0) { DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); ret = -EIO; goto out; @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp) return ret; } - if (drm_dp_dpcd_readb(_dp->aux, DP_TEST_SINK_MISC, ) < 0) + if (intel_dp_dpcd_read_wake(_dp->aux, DP_TEST_SINK_MISC, + , 1) < 0) return -EIO; if (!(buf & DP_TEST_CRC_SUPPORTED)) @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp) intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK; - if (drm_dp_dpcd_readb(_dp->aux, DP_TEST_SINK, ) < 0) + if (intel_dp_dpcd_read_wake(_dp->aux, DP_TEST_SINK, , 1) < 0) return -EIO; hsw_disable_ips(intel_crtc); @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) do { intel_wait_for_vblank(dev, intel_crtc->pipe); - if (drm_dp_dpcd_readb(_dp->aux, - DP_TEST_SINK_MISC, ) < 0) { + if (intel_dp_dpcd_read_wake(_dp->aux, + DP_TEST_SINK_MISC, , 1) < 0) { ret = -EIO; goto stop; } @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) if (count == 0) intel_dp->sink_crc.last_count = 0; - if (drm_dp_dpcd_read(_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) { + if (intel_dp_dpcd_read_wake(_dp->aux, DP_TEST_CRC_R_CR, + crc, 6) < 0) { ret = -EIO; goto stop; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.
On 10/21/2015 7:54 PM, Vivi, Rodrigo wrote: On Wed, 2015-10-21 at 12:19 +0300, Ville Syrjälä wrote: On Wed, Oct 21, 2015 at 10:28:51AM -0700, Rodrigo Vivi wrote: EBUSY retries are already in place at drm level. We don't need to replicate the job here. Signed-off-by: Rodrigo Vivi--- drivers/gpu/drm/i915/intel_dp.c | 20 ++-- 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 09bdd94..d054129 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -818,24 +818,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, intel_aux_display_runtime_get(dev_priv); - /* Try to wait for any previous AUX channel activity */ - for (try = 0; try < 3; try++) { - status = I915_READ_NOTRACE(ch_ctl); - if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) - break; - msleep(1); The dp helper doesn't sleep at all between the retries, so dropping the sleep from here may change the behaviour. hm indeed... what about + msleep(1); + ret = -EBUSY + goto out; ? the current code is one of few test cases that pass during compliance testing so please confirm this change does not break that :). regards, Sivakumar - } - - if (try == 3) { - static u32 last_status = -1; - const u32 status = I915_READ(ch_ctl); - - if (status != last_status) { - WARN(1, "dp_aux_ch not started status 0x%08x\n", -status); - last_status = status; - } - + status = I915_READ_NOTRACE(ch_ctl); + if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) { ret = -EBUSY; goto out; } -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use dpcd read wake for sink crc calls.
On 10/22/2015 1:44 AM, Damien Lespiau wrote: On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote: On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: On Mon, 2015-08-24 at 19:54 +, Zanoni, Paulo R wrote: Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: Let's use a native read with retry as suggested per spec to fix Sink CRC on SKL when PSR is enabled. With PSR enabled panel is probably taking more time to wake and dpcd read is faling. Does this commit actually fix any known problem with Sink CRC? Or is it just a try? It would be nice to have this clarified in the commit message. It was just a try but that made sink crc working on my SKL when PSR is enabled. nothing much to add... SKL has new register AUX_MUTEX which should be used when accessing dpcd on edp. just searched the nightly code and could not find it. it might be the reason for random dpcd failures reported in the other thread. We had patches for that back in December 2013 :) The feedback from Art was: The non-software aux users are PSR/SRD and GTC. Better leave out the mutex for now. Hardware is going to try do the arbitration itself. I expect you will then need to increase any software timeout you may have. Do you know if anything has changed since then? Not sure, it is in the bspec sequence to use AUX hence forwarded. Art might be the right person to contact :). it might be due to some minor DPCD access issues we observed in BDW when PSR was enabled. regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/15] drm/i915: Don't pass *DP around to link training functions
On 10/19/2015 2:26 PM, Ander Conselvan De Oliveira wrote: On Mon, 2015-10-19 at 10:15 +0530, Thulasimani, Sivakumar wrote: On 10/5/2015 12:31 PM, Ander Conselvan de Oliveira wrote: It just makes the code more confusing, so just reference intel_dp_>DP directly. The old behavior of not updating the value in intel_dp if link training fail is preserved by saving the previous value of DP in the stack and restoring the old value in case of failure. Signed-off-by: Ander Conselvan de Oliveira < ander.conselvan.de.olive...@intel.com> -- I'm not sure the old behavior is correct, but to err in the side of caution I tried not to change it. --- drivers/gpu/drm/i915/intel_dp.c | 66 - 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c420edf..391a367 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3601,7 +3601,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP) static bool intel_dp_set_link_train(struct intel_dp *intel_dp, - uint32_t *DP, uint8_t dp_train_pat) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3610,9 +3609,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, uint8_t buf[sizeof(intel_dp->train_set) + 1]; int ret, len; - _intel_dp_set_link_train(intel_dp, DP, dp_train_pat); + _intel_dp_set_link_train(intel_dp, _dp->DP, dp_train_pat); - I915_WRITE(intel_dp->output_reg, *DP); + I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); buf[0] = dp_train_pat; @@ -3633,17 +3632,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, } static bool -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, +intel_dp_reset_link_train(struct intel_dp *intel_dp, uint8_t dp_train_pat) { if (!intel_dp->train_set_valid) memset(intel_dp->train_set, 0, sizeof(intel_dp ->train_set)); - intel_dp_set_signal_levels(intel_dp, DP); - return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); + intel_dp_set_signal_levels(intel_dp, _dp->DP); + return intel_dp_set_link_train(intel_dp, dp_train_pat); } static bool -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, +intel_dp_update_link_train(struct intel_dp *intel_dp, const uint8_t link_status[DP_LINK_STATUS_SIZE]) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3652,9 +3651,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, int ret; intel_get_adjust_train(intel_dp, link_status); - intel_dp_set_signal_levels(intel_dp, DP); + intel_dp_set_signal_levels(intel_dp, _dp->DP); - I915_WRITE(intel_dp->output_reg, *DP); + I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); ret = drm_dp_dpcd_write(_dp->aux, DP_TRAINING_LANE0_SET, @@ -3695,7 +3694,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp) } /* Enable corresponding port and start training pattern 1 */ -static void +static bool intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) { struct drm_encoder *encoder = _to_dig_port(intel_dp) ->base.base; @@ -3703,9 +3702,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) int i; uint8_t voltage; int voltage_tries, loop_tries; - uint32_t DP = intel_dp->DP; uint8_t link_config[2]; uint8_t link_bw, rate_select; + uint8_t link_status[DP_LINK_STATUS_SIZE]; if (HAS_DDI(dev)) intel_ddi_prepare_link_retrain(encoder); @@ -3727,22 +3726,20 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) link_config[1] = DP_SET_ANSI_8B10B; drm_dp_dpcd_write(_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2); - DP |= DP_PORT_EN; + intel_dp->DP |= DP_PORT_EN; /* clock recovery */ - if (!intel_dp_reset_link_train(intel_dp, , + if (!intel_dp_reset_link_train(intel_dp, DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABLE)) { DRM_ERROR("failed to enable link training\n"); - return; + return false; } voltage = 0xff; voltage_tries = 0; loop_tries = 0; for (;;) { - uint8_t link_status[DP_LINK_STATUS_SIZE]; - drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd); if (!intel_dp_get_link_status(intel_dp, link_status)) { DRM_ERROR("faile
Re: [Intel-gfx] [PATCH 02/15] drm/i915: Don't pass *DP around to link training functions
On 10/5/2015 12:31 PM, Ander Conselvan de Oliveira wrote: It just makes the code more confusing, so just reference intel_dp_>DP directly. The old behavior of not updating the value in intel_dp if link training fail is preserved by saving the previous value of DP in the stack and restoring the old value in case of failure. Signed-off-by: Ander Conselvan de Oliveira-- I'm not sure the old behavior is correct, but to err in the side of caution I tried not to change it. --- drivers/gpu/drm/i915/intel_dp.c | 66 - 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c420edf..391a367 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3601,7 +3601,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP) static bool intel_dp_set_link_train(struct intel_dp *intel_dp, - uint32_t *DP, uint8_t dp_train_pat) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3610,9 +3609,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, uint8_t buf[sizeof(intel_dp->train_set) + 1]; int ret, len; - _intel_dp_set_link_train(intel_dp, DP, dp_train_pat); + _intel_dp_set_link_train(intel_dp, _dp->DP, dp_train_pat); - I915_WRITE(intel_dp->output_reg, *DP); + I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); buf[0] = dp_train_pat; @@ -3633,17 +3632,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, } static bool -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, +intel_dp_reset_link_train(struct intel_dp *intel_dp, uint8_t dp_train_pat) { if (!intel_dp->train_set_valid) memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); - intel_dp_set_signal_levels(intel_dp, DP); - return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); + intel_dp_set_signal_levels(intel_dp, _dp->DP); + return intel_dp_set_link_train(intel_dp, dp_train_pat); } static bool -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, +intel_dp_update_link_train(struct intel_dp *intel_dp, const uint8_t link_status[DP_LINK_STATUS_SIZE]) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -3652,9 +3651,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, int ret; intel_get_adjust_train(intel_dp, link_status); - intel_dp_set_signal_levels(intel_dp, DP); + intel_dp_set_signal_levels(intel_dp, _dp->DP); - I915_WRITE(intel_dp->output_reg, *DP); + I915_WRITE(intel_dp->output_reg, intel_dp->DP); POSTING_READ(intel_dp->output_reg); ret = drm_dp_dpcd_write(_dp->aux, DP_TRAINING_LANE0_SET, @@ -3695,7 +3694,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp) } /* Enable corresponding port and start training pattern 1 */ -static void +static bool intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) { struct drm_encoder *encoder = _to_dig_port(intel_dp)->base.base; @@ -3703,9 +3702,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) int i; uint8_t voltage; int voltage_tries, loop_tries; - uint32_t DP = intel_dp->DP; uint8_t link_config[2]; uint8_t link_bw, rate_select; + uint8_t link_status[DP_LINK_STATUS_SIZE]; if (HAS_DDI(dev)) intel_ddi_prepare_link_retrain(encoder); @@ -3727,22 +3726,20 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) link_config[1] = DP_SET_ANSI_8B10B; drm_dp_dpcd_write(_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2); - DP |= DP_PORT_EN; + intel_dp->DP |= DP_PORT_EN; /* clock recovery */ - if (!intel_dp_reset_link_train(intel_dp, , + if (!intel_dp_reset_link_train(intel_dp, DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABLE)) { DRM_ERROR("failed to enable link training\n"); - return; + return false; } voltage = 0xff; voltage_tries = 0; loop_tries = 0; for (;;) { - uint8_t link_status[DP_LINK_STATUS_SIZE]; - drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd); if (!intel_dp_get_link_status(intel_dp, link_status)) { DRM_ERROR("failed to get link status\n"); @@ -3762,11 +3759,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) DRM_DEBUG_KMS("clock recovery not ok, reset"); /* clear the flag as we are not
Re: [Intel-gfx] [PATCH 7/8] drm/i915: Move max voltage and pre emphasis to intel_dp_link_training.c
As an FYI, both of these functions need to be rewritten when we want the code to be compliant to DP spec. We should read the pre-emphasis given by the panel and if vwing exeeds max value we should use the max vswing supported for that pre-emphasis but current code is opposite of that. you can read more detailed logic in section "3.5.1.2 Link Training" in DP Spec. by the way same is true for much of the link training logic and related functions i don't know if such rewriting should be done after fixing those or before :(. regards, Sivakumar. On 9/8/2015 6:31 PM, Ville Syrjälä wrote: On Tue, Sep 08, 2015 at 03:27:58PM +0300, Ander Conselvan de Oliveira wrote: So link training tests can use real hardware limits. These need to be kept in sync with the _signal_levels() functions, so moving them to a separate file is a bit questionable. I suggest that we should attempt to restructure this information as some kind of tables, from which we can look up the max values as well as the hardware specific values for each setting. That of course won't solve your problem. So far I don't know what your test even does and why does it need this information, so I guess I'll need to read on... --- drivers/gpu/drm/i915/intel_dp.c | 99 --- drivers/gpu/drm/i915/intel_dp_link_training.c | 92 + drivers/gpu/drm/i915/intel_drv.h | 11 +-- 3 files changed, 99 insertions(+), 103 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 5778059..da87aef 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -111,13 +111,6 @@ static bool is_edp(struct intel_dp *intel_dp) return intel_dig_port->base.type == INTEL_OUTPUT_EDP; } -static struct drm_device *intel_dp_to_dev(struct intel_dp *intel_dp) -{ - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); - - return intel_dig_port->base.base.dev; -} - static struct intel_dp *intel_attached_dp(struct drm_connector *connector) { return enc_to_intel_dp(_attached_encoder(connector)->base); @@ -3054,98 +3047,6 @@ intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_ DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE; } -/* These are source-specific values. */ -uint8_t -intel_dp_voltage_max(struct intel_dp *intel_dp) -{ - struct drm_device *dev = intel_dp_to_dev(intel_dp); - struct drm_i915_private *dev_priv = dev->dev_private; - enum port port = dp_to_dig_port(intel_dp)->port; - - if (IS_BROXTON(dev)) - return DP_TRAIN_VOLTAGE_SWING_LEVEL_3; - else if (INTEL_INFO(dev)->gen >= 9) { - if (dev_priv->edp_low_vswing && port == PORT_A) - return DP_TRAIN_VOLTAGE_SWING_LEVEL_3; - return DP_TRAIN_VOLTAGE_SWING_LEVEL_2; - } else if (IS_VALLEYVIEW(dev)) - return DP_TRAIN_VOLTAGE_SWING_LEVEL_3; - else if (IS_GEN7(dev) && port == PORT_A) - return DP_TRAIN_VOLTAGE_SWING_LEVEL_2; - else if (HAS_PCH_CPT(dev) && port != PORT_A) - return DP_TRAIN_VOLTAGE_SWING_LEVEL_3; - else - return DP_TRAIN_VOLTAGE_SWING_LEVEL_2; -} - -uint8_t -intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing) -{ - struct drm_device *dev = intel_dp_to_dev(intel_dp); - enum port port = dp_to_dig_port(intel_dp)->port; - - if (INTEL_INFO(dev)->gen >= 9) { - switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) { - case DP_TRAIN_VOLTAGE_SWING_LEVEL_0: - return DP_TRAIN_PRE_EMPH_LEVEL_3; - case DP_TRAIN_VOLTAGE_SWING_LEVEL_1: - return DP_TRAIN_PRE_EMPH_LEVEL_2; - case DP_TRAIN_VOLTAGE_SWING_LEVEL_2: - return DP_TRAIN_PRE_EMPH_LEVEL_1; - case DP_TRAIN_VOLTAGE_SWING_LEVEL_3: - return DP_TRAIN_PRE_EMPH_LEVEL_0; - default: - return DP_TRAIN_PRE_EMPH_LEVEL_0; - } - } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { - switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) { - case DP_TRAIN_VOLTAGE_SWING_LEVEL_0: - return DP_TRAIN_PRE_EMPH_LEVEL_3; - case DP_TRAIN_VOLTAGE_SWING_LEVEL_1: - return DP_TRAIN_PRE_EMPH_LEVEL_2; - case DP_TRAIN_VOLTAGE_SWING_LEVEL_2: - return DP_TRAIN_PRE_EMPH_LEVEL_1; - case DP_TRAIN_VOLTAGE_SWING_LEVEL_3: - default: - return DP_TRAIN_PRE_EMPH_LEVEL_0; - } - } else if (IS_VALLEYVIEW(dev)) { - switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) { - case DP_TRAIN_VOLTAGE_SWING_LEVEL_0: -
Re: [Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling)
Hi All, I went through Ville's changes and it seems to be lacking support for our one main user experience requirement in Android. Which is as follows Ux Restrictions: Flash/flicker should be avoided as much as possible( i.e during unplug of HDMI avoid immediately lowering the CD clock after hotunplug since it will result in flicker on LFP ). The display will be turned off eventually (especially considering Android) and would be good to change the CD clock at this point in time rather than immediately on hot unplug. So we have to add to current implementation to modify the behavior or go with the HWC+driver logic described below. regards, Sivakumar -Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Thulasimani, Sivakumar Sent: Friday, January 16, 2015 9:56 AM To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org; Purushothaman, Vijay A Subject: Re: [Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling) Thanks for the pointer, let me sync with Ville and get back. regards, Sivakumar -Original Message- From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Friday, January 16, 2015 1:08 AM To: Thulasimani, Sivakumar Cc: intel-gfx@lists.freedesktop.org; Purushothaman, Vijay A; Syrjala, Ville Subject: Re: [Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling) You realize that Ville has implemented all this already and his patches are only waiting for review? -Daniel On Thu, Jan 15, 2015 at 2:50 PM, Thulasimani, Sivakumar sivakumar.thulasim...@intel.com wrote: Hi, I’ll be working on implementing Dynamic Voltage Frequency Scaling in i915, whose rough proposal is provided below. Please go through the options and provide your feedback. What is DVFS ? Any SKU is capable of running at more than one display CD clock value but is configured to a default value during boot and is left untouched afterwards. Lowering this Display CD clock will result in better power savings while raising this will result in capability to support larger resolution displays. So the best scenario is to always detect the attached displays and adjust the CD Clock to the minimum required by the displays. DVFS is the process of dynamically changing the display CD clock based on attached displays. Usecases: ( considers low resolution LFP is used with 4K HDMI panel) Ø EDP (19x10) LFP panel alone is present at boot and can be driven with lower CD clock, thus saving power. Ø EDP (19x10) LFP panel was present at boot and 4K HDMI display is hot plugged in to the system. CD clock can be increased to the required value and then we can drive 4K panel. Thus save power while only LFP is in use but also provide the ability to support 4K displays. Ø 4K HDMI display is unplugged from the system, CD clock can now be lowered to the value required to drive LFP alone to save power. Ø Boot with LFP and 4K HDMI connected, the CD clock will be programmed to the value required to drive 4K HDMI. Technical Restrictions: Ø DVFS can be performed only when all displays are turned off (pipe/port/plane,etc) Ux Restrictions: Ø Flash/flicker should be avoided as much as possible( i.e during unplug of HDMI avoid immediately lowering the CD clock after hotunplug since it will result in flicker on LFP ) Possible Solutions : Ø Policy + implementation in driver o Driver will set the cd clock during boot based on attached panels o Post boot driver has to track each modeset for CD clock and resolution support. o If any change in CD clock is needed, driver has to disable all displays , change the cd clock and enable back all displays o Pros: · Changes are contained within driver o Cons: · Complexity increases within driver · User land/ HWC will be blind to internal operations Ø Policy in HWC and implementation in driver o Driver will set the cd clock during boot based on attached panels o Post boot, driver will fail any modeset that cannot be supported o HWC will be responsible for disabling all displays, issuing IOCTL to change CD clock and enabling required displays o Pros: · Driver implementation will be simple and clean · User land/HWC will be aware of operations and handle any special cases such as video playback o Cons: · Solution requires HWC Driver changes. Our recommendation is for HWC + Driver changes considering our primary target of Android and the pros/cons mentioned there. Sorry for the long mail J, Please provide your feedback or questions on this. regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel
Re: [Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling)
Thanks for the suggestion Daniel, this seems simpler solution to the problem. I'll check for corner cases and get back if any more changes are needed. regards, Sivakumar -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, February 11, 2015 2:24 PM To: Thulasimani, Sivakumar Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Purushothaman, Vijay A Subject: Re: [Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling) On Wed, Feb 11, 2015 at 09:51:53AM +0100, Daniel Vetter wrote: On Wed, Feb 11, 2015 at 08:32:59AM +, Thulasimani, Sivakumar wrote: Hi All, I went through Ville's changes and it seems to be lacking support for our one main user experience requirement in Android. Which is as follows Ux Restrictions: Flash/flicker should be avoided as much as possible( i.e during unplug of HDMI avoid immediately lowering the CD clock after hotunplug since it will result in flicker on LFP ). The display will be turned off eventually (especially considering Android) and would be good to change the CD clock at this point in time rather than immediately on hot unplug. So we have to add to current implementation to modify the behavior or go with the HWC+driver logic described below. Hm yeah. But for upstreaming can we just start with Ville's patch series to get things going? Thinking about this a bit more it's easy to implement your desired behaviour just in hwc: - On unplug of hdmi only do a dpms off for that hdmi output/crtc. That way the kernel will keep all allocations/requirements and hence also cdclock high. - When you power off the panel with dpms off, then you can do the setCrtc(off) to release resources and clock down cdclock. So no real changes required on the kernel side imo. -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
Re: [Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling)
Can anyone point me to the patches if any shared by Ville ? I am not able to contact him. regards, Sivakumar -Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Thulasimani, Sivakumar Sent: Friday, January 16, 2015 9:56 AM To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org; Purushothaman, Vijay A Subject: Re: [Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling) Thanks for the pointer, let me sync with Ville and get back. regards, Sivakumar -Original Message- From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Friday, January 16, 2015 1:08 AM To: Thulasimani, Sivakumar Cc: intel-gfx@lists.freedesktop.org; Purushothaman, Vijay A; Syrjala, Ville Subject: Re: [Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling) You realize that Ville has implemented all this already and his patches are only waiting for review? -Daniel On Thu, Jan 15, 2015 at 2:50 PM, Thulasimani, Sivakumar sivakumar.thulasim...@intel.com wrote: Hi, I’ll be working on implementing Dynamic Voltage Frequency Scaling in i915, whose rough proposal is provided below. Please go through the options and provide your feedback. What is DVFS ? Any SKU is capable of running at more than one display CD clock value but is configured to a default value during boot and is left untouched afterwards. Lowering this Display CD clock will result in better power savings while raising this will result in capability to support larger resolution displays. So the best scenario is to always detect the attached displays and adjust the CD Clock to the minimum required by the displays. DVFS is the process of dynamically changing the display CD clock based on attached displays. Usecases: ( considers low resolution LFP is used with 4K HDMI panel) Ø EDP (19x10) LFP panel alone is present at boot and can be driven with lower CD clock, thus saving power. Ø EDP (19x10) LFP panel was present at boot and 4K HDMI display is hot plugged in to the system. CD clock can be increased to the required value and then we can drive 4K panel. Thus save power while only LFP is in use but also provide the ability to support 4K displays. Ø 4K HDMI display is unplugged from the system, CD clock can now be lowered to the value required to drive LFP alone to save power. Ø Boot with LFP and 4K HDMI connected, the CD clock will be programmed to the value required to drive 4K HDMI. Technical Restrictions: Ø DVFS can be performed only when all displays are turned off (pipe/port/plane,etc) Ux Restrictions: Ø Flash/flicker should be avoided as much as possible( i.e during unplug of HDMI avoid immediately lowering the CD clock after hotunplug since it will result in flicker on LFP ) Possible Solutions : Ø Policy + implementation in driver o Driver will set the cd clock during boot based on attached panels o Post boot driver has to track each modeset for CD clock and resolution support. o If any change in CD clock is needed, driver has to disable all displays , change the cd clock and enable back all displays o Pros: · Changes are contained within driver o Cons: · Complexity increases within driver · User land/ HWC will be blind to internal operations Ø Policy in HWC and implementation in driver o Driver will set the cd clock during boot based on attached panels o Post boot, driver will fail any modeset that cannot be supported o HWC will be responsible for disabling all displays, issuing IOCTL to change CD clock and enabling required displays o Pros: · Driver implementation will be simple and clean · User land/HWC will be aware of operations and handle any special cases such as video playback o Cons: · Solution requires HWC Driver changes. Our recommendation is for HWC + Driver changes considering our primary target of Android and the pros/cons mentioned there. Sorry for the long mail J, Please provide your feedback or questions on this. regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] proposal for DVFS (Dynamic Voltage Frequency Scaling)
Hi, I'll be working on implementing Dynamic Voltage Frequency Scaling in i915, whose rough proposal is provided below. Please go through the options and provide your feedback. What is DVFS ? Any SKU is capable of running at more than one display CD clock value but is configured to a default value during boot and is left untouched afterwards. Lowering this Display CD clock will result in better power savings while raising this will result in capability to support larger resolution displays. So the best scenario is to always detect the attached displays and adjust the CD Clock to the minimum required by the displays. DVFS is the process of dynamically changing the display CD clock based on attached displays. Usecases: ( considers low resolution LFP is used with 4K HDMI panel) ? EDP (19x10) LFP panel alone is present at boot and can be driven with lower CD clock, thus saving power. ? EDP (19x10) LFP panel was present at boot and 4K HDMI display is hot plugged in to the system. CD clock can be increased to the required value and then we can drive 4K panel. Thus save power while only LFP is in use but also provide the ability to support 4K displays. ? 4K HDMI display is unplugged from the system, CD clock can now be lowered to the value required to drive LFP alone to save power. ? Boot with LFP and 4K HDMI connected, the CD clock will be programmed to the value required to drive 4K HDMI. Technical Restrictions: ? DVFS can be performed only when all displays are turned off (pipe/port/plane,etc) Ux Restrictions: ? Flash/flicker should be avoided as much as possible( i.e during unplug of HDMI avoid immediately lowering the CD clock after hotunplug since it will result in flicker on LFP ) Possible Solutions : ? Policy + implementation in driver o Driver will set the cd clock during boot based on attached panels o Post boot driver has to track each modeset for CD clock and resolution support. o If any change in CD clock is needed, driver has to disable all displays , change the cd clock and enable back all displays o Pros: * Changes are contained within driver o Cons: * Complexity increases within driver * User land/ HWC will be blind to internal operations ? Policy in HWC and implementation in driver o Driver will set the cd clock during boot based on attached panels o Post boot, driver will fail any modeset that cannot be supported o HWC will be responsible for disabling all displays, issuing IOCTL to change CD clock and enabling required displays o Pros: * Driver implementation will be simple and clean * User land/HWC will be aware of operations and handle any special cases such as video playback o Cons: * Solution requires HWC Driver changes. Our recommendation is for HWC + Driver changes considering our primary target of Android and the pros/cons mentioned there. Sorry for the long mail :), Please provide your feedback or questions on this. regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx