Re: [PATCH 1/6] drm/i915: Extract out gamma table and CSC to their own file

From: Matt Roper <matthew.d.roper@intel.com>
To: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/6] drm/i915: Extract out gamma table and CSC to their own file
Date: Wed, 17 Feb 2016 17:51:24 -0800	[thread overview]
Message-ID: <20160218015124.GY27772@intel.com> (raw)
In-Reply-To: <1454339917-31569-2-git-send-email-lionel.g.landwerlin@intel.com>

On Mon, Feb 01, 2016 at 03:18:32PM +0000, Lionel Landwerlin wrote:
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

Given the patch headline and lack of commit message body, I expected
this to be pretty much a straight migration of the code between files
without other changes (aside from [un]static'ing of functions and such).
But it looks like there are a handful of non-movement changes here as
well.  Aside from the function renames (which might be worth noting in
the commit message), there are a couple of changes to gamma LUT loading
that look like they're a functional change on some platforms.  Assuming
those were intentional changes, those should probably be described in
the commit message, or else split into a separate patch so that they can
be reviewed more easily.

...
> +/** Loads the palette/gamma unit for the CRTC with the prepared values on  */

Preexisting mistake from the original code, but this isn't actually a
kerneldoc comment, so you might want to make this just a "/*" opener.

> +static void i9xx_load_legacy_gamma_lut(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	enum pipe pipe = intel_crtc->pipe;
> +	int i;
> +	bool reenable_ips = false;
> +
> +	if (HAS_GMCH_DISPLAY(dev)) {
> +		if (intel_crtc->config->has_dsi_encoder)
> +			assert_dsi_pll_enabled(dev_priv);
> +		else
> +			assert_pll_enabled(dev_priv, pipe);
> +	}
> +
> +	/* Workaround : Do not read or write the pipe palette/gamma data while
> +	 * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> +	 */
> +	if (IS_HASWELL(dev) && intel_crtc->config->ips_enabled &&
> +	    ((I915_READ(GAMMA_MODE(pipe)) & GAMMA_MODE_MODE_MASK) ==
> +	     GAMMA_MODE_MODE_SPLIT)) {
> +		hsw_disable_ips(intel_crtc);
> +		reenable_ips = true;
> +	}
> +
> +	for (i = 0; i < 256; i++) {
> +		uint32_t word = (intel_crtc->lut_r[i] << 16) |
> +			(intel_crtc->lut_g[i] << 8) |
> +			intel_crtc->lut_b[i];
> +		if (HAS_GMCH_DISPLAY(dev))
> +			I915_WRITE(PALETTE(pipe, i), word);
> +		else
> +			I915_WRITE(LGC_PALETTE(pipe, i), word);
> +	}

This loop was re-written / re-organized from the original code, but
looks like it should still accomplish the same thing.

> +
> +	I915_WRITE(GAMMA_MODE(intel_crtc->pipe), GAMMA_MODE_MODE_8BIT);

This wasn't in the original function, but rather moved from
haswell_set_pipeconf() so it was only called on a smaller subset of
platforms in the past.  Is that an intentional change, or a mistake?
You also seem to have dropped the POSTING_READ() that followed it.

Matt

> +
> +	if (reenable_ips)
> +		hsw_enable_ips(intel_crtc);
> +}
> +
> +void intel_color_load_gamma_lut(struct drm_crtc *crtc)
> +{
> +	/* The clocks have to be on to load the palette. */
> +	if (!crtc->state->active)
> +		return;
> +
> +	i9xx_load_legacy_gamma_lut(crtc);
> +}

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx