Re: [PATCH] drm/i915: Skip DDI PLL selection for DSI

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 09 Feb 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Feb 09, 2016 at 01:16:18PM +0530, Thulasimani, Sivakumar wrote:
>> 
>> 
>> On 2/9/2016 12:02 PM, Jani Nikula wrote:
>> > On Tue, 09 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani@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@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@intel.com>
>
> Another idea would be to use the clock_set thing to skip it, but
> I think  historically that has only been used to skip the PLL
> calculations, not the PLL selection. So might be it would just confuse
> things more.

I just pushed this one. Thanks for the patch and review.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx