On Mon, Feb 01, 2016 at 03:18:32PM +0000, Lionel Landwerlin wrote:
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
