Re: [PATCH v4 5/6] drm/i915: enable scrambling

From: "Sharma, Shashank" <shashank.sharma@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Ander Conselvan De Oliveira" <conselvan2@gmail.com>
Cc: jose.abreu@synopsys.com, daniel.vetter@intel.com,
	intel-gfx@lists.freedesktop.org, treding@nvidia.com,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 5/6] drm/i915: enable scrambling
Date: Thu, 23 Feb 2017 20:46:05 +0530	[thread overview]
Message-ID: <a0ece8f3-96e6-e85e-cbdc-322fe7ae79f6@intel.com> (raw)
In-Reply-To: <20170223132135.GV31595@intel.com>


[-- Attachment #1.1: Type: text/plain, Size: 2985 bytes --]


>> Think about two situations where:-
>> - Monitor supports scrambling and scdc, but we will not enable it, as
>> the current mode is 1080P@148 MHz
>> - Monitor supports scrambling and scdc, and we will enable it, as the
>> current mode is 4k@596 Mhz
>>
>> To differentiate between these two, we have:
>> config->hdmi_scrambling which shows scrambling enabled on monitor by source
>> display_info->hdmi.scdc->scrambling which indicates monitor supports
>> scrambling
>>
>> Does it make sense ? Or you prefer some changes here ?
> Is there any harm in disabling scrambling twice? If not I'd say just disable it
> on every modeset if it is not needed. Then there's no need to know the previous
> state at the moment we actually enable/disable it.

> Agreed. Just explicitly set the state to what we want every time.

- Cool, so I would load the crtc_state only once (in compute_config), and then disable it every time during ddi_disable.
Thanks for this clarification guys, I will send a follow up patch series addressing all the comments soon.

Regards
Shashank

On 2/23/2017 6:51 PM, Ville Syrjälä wrote:
>>> > >Actually design is slightly different. The state's hdmi_scrambling/clock
>>> > >bool's indicate that the sink's
>>> > >scrambling/high_clock_ratio is enabled/set by source (and needs to be
>>> > >disabled), whereas connecotr's->display_info->scdc.scrambling
>>> > >is to indicate monitor's capability to support scrambling and scdc
>> >
>> >The crtc_state shouldn't be changed to represent changes in the hardware state.
>> >It is computed during the atomic check phase, and after that it should represent
>> >the state that needs to be commited. Perhaps the code in hdmi_compute_config()
>> >just needs to take the previous state into account?
> The previous state is irrelevant. We just need to compute these things
> based on what we're going to do next. And this stuff doesn't really
> track the state of the sink, rather it tracks the state of the source.
> The sink state just follows along to match. So in the readout path we
> just want to read out the state of the source.
>
>> >
>>> > >
>>> > >Think about two situations where:-
>>> > >- Monitor supports scrambling and scdc, but we will not enable it, as
>>> > >the current mode is 1080P@148 MHz
>>> > >- Monitor supports scrambling and scdc, and we will enable it, as the
>>> > >current mode is 4k@596 Mhz
>>> > >
>>> > >To differentiate between these two, we have:
>>> > >config->hdmi_scrambling which shows scrambling enabled on monitor by source
>>> > >display_info->hdmi.scdc->scrambling which indicates monitor supports
>>> > >scrambling
>>> > >
>>> > >Does it make sense ? Or you prefer some changes here ?
>> >
>> >Is there any harm in disabling scrambling twice? If not I'd say just disable it
>> >on every modeset if it is not needed. Then there's no need to know the previous
>> >state at the moment we actually enable/disable it.
> Agreed. Just explicitly set the state to what we want every time.


[-- Attachment #1.2: Type: text/html, Size: 5155 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel