[Intel-gfx] [v5 2/6] drm/i915: Sanitize crtc gamma mode

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, January 11, 2019 4:08 AM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst at intel.com>; Syrjala, Ville <ville.syrjala at intel.com>; Sharma,
>Shashank <shashank.sharma at intel.com>
>Subject: Re: [v5 2/6] drm/i915: Sanitize crtc gamma mode
>
>On Tue, Jan 08, 2019 at 01:07:29PM +0530, Uma Shankar wrote:
>> Sanitize crtc gamma mode and update the mode in driver in case BIOS
>> has setup a different gamma mode as to what is expected by driver.
>> There is restriction on HSW platform not to read/write color LUT's if
>> ips is enabled. Handled the same accordingly.
>>
>> Credits-to: Matt Roper <matthew.d.roper at intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 696e6f5..03c8f68 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15401,6 +15401,23 @@ static void intel_sanitize_crtc(struct intel_crtc
>*crtc,
>>  		}
>>  	}
>>
>> +	/*
>> +	 * Sanitize gamma mode incase BIOS leaves it in SPLIT GAMMA MODE
>> +	 * Workaround HSW : Do not read or write the pipe palette/gamma data
>> +	 * while GAMMA_MODE is configured for split gamma and IPS_CTL has
>IPS
>> +	 * enabled.
>> +	 */
>
>The other thing that might be worth noting here is that we don't actually try to
>read out the LUT's and CTM that the BIOS setup, so at the moment they stick
>around for a while until the get unexpectedly
>clobbered by a subsequent modeset or fastset.   The change here will
>basically force them to be reset to standard/linear values at startup.
>
>Maybe in the future we'll try to actually read out and preserve the contents of the
>actual LUT's and CTM that the BIOS had setup, but we don't do that yet today, so
>the change here at least makes the behavior a little bit more consistent than what
>it has been.
>
>Up to you whether you want to try to describe that in either the comment and/or
>commit message.

Sure Matt, I will update the commit message to reflect this as well.

>> +	if (IS_HASWELL(dev_priv)) {
>> +		if (crtc_state->ips_enabled)
>
>It looks like both hsw_disable_ips() and hsw_enable_ips() have this test inside of
>them already, so we can just call them unconditionally here.
>

Yes, this can be dropped. Will do that.

>Aside from that,
>
>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
>

Thanks Matt for the review and valuable comments.

Regards,
Uma Shankar
>> +			hsw_disable_ips(crtc_state);
>> +
>> +		intel_color_set_csc(crtc_state);
>> +		intel_color_load_luts(crtc_state);
>> +
>> +		if (crtc_state->ips_enabled)
>> +			hsw_enable_ips(crtc_state);
>> +	}
>> +
>>  	/* Adjust the state of the output pipe according to whether we
>>  	 * have active connectors/encoders. */
>>  	if (crtc_state->base.active && !intel_crtc_has_encoders(crtc))
>> --
>> 1.9.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795