From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Subject: Re: [PATCH v4 5/6] drm/i915: enable scrambling Date: Thu, 23 Feb 2017 20:46:05 +0530 Message-ID: References: <1487769511-17359-1-git-send-email-shashank.sharma@intel.com> <1487769511-17359-6-git-send-email-shashank.sharma@intel.com> <20170222172954.GM31595@intel.com> <548317e5-2fab-9832-142f-581be31aac39@intel.com> <1487836997.2361.2.camel@gmail.com> <1487855260.3385.6.camel@gmail.com> <20170223132135.GV31595@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1606165504==" Return-path: In-Reply-To: <20170223132135.GV31595@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Ander Conselvan De Oliveira Cc: jose.abreu@synopsys.com, daniel.vetter@intel.com, intel-gfx@lists.freedesktop.org, treding@nvidia.com, dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============1606165504== Content-Type: multipart/alternative; boundary="------------0C516986A82B72EC445F9946" This is a multi-part message in MIME format. --------------0C516986A82B72EC445F9946 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit >> 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. --------------0C516986A82B72EC445F9946 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 8bit 
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.

--------------0C516986A82B72EC445F9946-- --===============1606165504== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1606165504==--