[Intel-gfx] [PATCH] drm/i915: Skip DDI PLL selection for DSI

[Thulasimani, Sivakumar]

On 2/9/2016 12:02 PM, Jani Nikula wrote:
> On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani at intel.com> wrote:
>> On 2/5/2016 4:59 PM, Mika Kahola wrote:
>>> Skip DDI PLL selection if display type is DSI/MIPI.
>>>
>>> Signed-off-by: Mika Kahola <mika.kahola at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_display.c | 9 +++++++--
>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index d7de2a5..5da98b2 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -9902,8 +9902,13 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>>>    static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>>>    				      struct intel_crtc_state *crtc_state)
>>>    {
>>> -	if (!intel_ddi_pll_select(crtc, crtc_state))
>>> -		return -EINVAL;
>>> +	struct intel_encoder *intel_encoder =
>>> +		intel_ddi_get_crtc_new_encoder(crtc_state);
>>> +
>>> +	if (intel_encoder->type != INTEL_OUTPUT_DSI) {
>>> +		if (!intel_ddi_pll_select(crtc, crtc_state))
>>> +			return -EINVAL;
>>> +	}
>>>    
>> can this be moved inside bxt_ddi_pll_select ? we can avoid this check for
>> other platforms that also execute this function.
> I asked Mika to do it this way, but if you feel strongly about it I
> guess I could be persuaded otherwise too.
>
> My main point is, if we pass on DSI encoders to DDI functions in some
> cases but mostly not, it will muddy the waters and eventually people end
> up checking for "is dsi" all around DDI just because they can't be
> bothered to check if the functions are really called for DDI only or
> not. It's more of a maintainability concern than anything else.
>
> BR,
> Jani.
>
i am fine with this either way. i was thinking of avoid such checks
in other platforms where it is not needed but your concern of
too many is_dsi checks is valid as well.
with that i am fine with this change as is.
  Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani at intel.com>
>
>> regards,
>> Sivakumar
>>>    	crtc->lowfreq_avail = false;
>>>