Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 03/04/2015 02:15 AM, Matt Roper wrote:
> Current ILK-style watermark code assumes the primary plane and cursor
> plane are always enabled.  This assumption, along with the combination
> of two independent commits that got merged at the same time, results in
> a NULL dereference.  The offending commits are:
>
>          commit fd2d61341bf39d1054256c07d6eddd624ebc4241
>          Author: Matt Roper <matthew.d.roper@intel.com>
>          Date:   Fri Feb 27 10:12:01 2015 -0800
>
>              drm/i915: Use plane->state->fb in watermark code (v2)
>
> and
>
>          commit 0fda65680e92545caea5be7805a7f0a617fb6c20
>          Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>          Date:   Fri Feb 27 15:12:35 2015 +0000
>
>              drm/i915/skl: Update watermarks for Y tiling
>
> The first commit causes us to use the FB from plane->state->fb rather
> than the legacy plane->fb, which is updated a bit later in the process.
>
> The second commit includes a change that now triggers watermark
> reprogramming on primary plane enable/disable where we didn't have one
> before (which wasn't really correct, but we had been getting lucky
> because we always calculated as if the primary plane was on).
>
> Together, these two commits cause the watermark calculation to
> (properly) see plane->state->fb = NULL when we're in the process of
> disabling the primary plane.  However the existing watermark code
> assumes there's always a primary fb and tries to dereference it to find
> out pixel format / bpp information.
>
> The fix is to make ILK-style watermark calculation actually check the
> true status of primary & cursor planes and adjust our watermark logic
> accordingly.
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michael Leuchtenburg <michael@slashhome.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89388
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e710b43..93fd15f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1924,13 +1924,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
>   	p->active = true;
>   	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
>   	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> -	p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
> -	p->cur.bytes_per_pixel = 4;
> +
> +	if (crtc->primary->state->fb) {
> +		p->pri.enabled = true;
> +		p->pri.bytes_per_pixel =
> +			crtc->primary->state->fb->bits_per_pixel / 8;
> +	} else {
> +		p->pri.enabled = false;
> +		p->pri.bytes_per_pixel = 0;
> +	}
> +
> +	if (crtc->cursor->state->fb) {
> +		p->cur.enabled = true;
> +		p->cur.bytes_per_pixel = 4;
> +	} else {
> +		p->cur.enabled = false;
> +		p->cur.bytes_per_pixel = 0;
> +	}
>   	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
>   	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
> -	/* TODO: for now, assume primary and cursor planes are always enabled. */
> -	p->pri.enabled = true;
> -	p->cur.enabled = true;
>
>   	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>   		struct intel_plane *intel_plane = to_intel_plane(plane);
>

Hm there are other place which dereference crtc->primary->state->fb, 
like i9xx_update_wm and skl_compute_wm_pipe_parameters - they won't 
suffer the same problem?

Regards,

Tvrtko

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