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

["Sharma, Shashank" <shashank.sharma@xxxxxxxxx>]

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.

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