Re: [PATCH 04/14] drm/i915: check TRANSCODER_EDP on intel_modeset_setup_hw_state

From: Paulo Zanoni <przanoni@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 04/14] drm/i915: check TRANSCODER_EDP on intel_modeset_setup_hw_state
Date: Fri, 19 Oct 2012 16:30:46 -0300	[thread overview]
Message-ID: <CA+gsUGSpGSpAG8gv-QxZno_FcVcv4ev9vWmQn5hVf6ohz_dd+Q@mail.gmail.com> (raw)
In-Reply-To: <20121018220202.GF5309@phenom.ffwll.local>

Hi

2012/10/18 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Oct 18, 2012 at 06:21:34PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> We need to check if any of the pipes is using TRANSCODER_EDP.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> I wonder whether it doesn't make more sense for haswell to return the
> transcoder in the encoder->get_hw_state function, and then map that to the
> crtc with the intel_pipe_to_cpu_transcoder. That way we don't need to add
> a special-case for eDP.

I know we had some IRC discussions about this comment, but after this
I tried to implement your suggestion. It won't work unless we
completely rewrite a lot of code. For example, on
intel_modeset_setup_hw_state (the function this patch changes) we need
to know which transcoder is associated to the pipe before doing
I915_READ(PIPECONF(pipe)) (because PIPECONF(pipe) is wrong, we need
PIPECONF(cpu_transcoder), which comes later in the patch series), so
we will need to discover the encoder state before discovering the pipe
state, which is not what we do now. Also, having
dev_priv->transcoder_to_pipe_mapping is not as clean as having
pipe_to_transcoder_mapping because every pipe is always associated
with a transcoder, but not every transcoder is associated to a pipe,
so we'd need to keep checking for NULL on unused transcoders.

My first strategy to implement this code also tried to use
"for_each_transcoder" instead of "for_each_pipe" when checking the
pipe state, but it added even more complexity to the function.

I really believe my approach here is as KISS as we can get. A single
special haswell case here prevents a lot of "is this cpu transcoder
being used by some pipe?" checks in a lot of places.

> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 73ce007..827c5ba 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -8551,6 +8551,31 @@ void intel_modeset_setup_hw_state(struct drm_device *dev)
>>       struct intel_encoder *encoder;
>>       struct intel_connector *connector;
>>
>> +     if (IS_HASWELL(dev)) {
>> +             tmp = I915_READ(DDI_FUNC_CTL(TRANSCODER_EDP));
>> +
>> +             if (tmp & TRANS_DDI_FUNC_ENABLE) {
>> +                     switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
>> +                     case TRANS_DDI_EDP_INPUT_A_ON:
>> +                     case TRANS_DDI_EDP_INPUT_A_ONOFF:
>> +                             pipe = PIPE_A;
>> +                             break;
>> +                     case TRANS_DDI_EDP_INPUT_B_ONOFF:
>> +                             pipe = PIPE_B;
>> +                             break;
>> +                     case TRANS_DDI_EDP_INPUT_C_ONOFF:
>> +                             pipe = PIPE_C;
>> +                             break;
>> +                     }
>> +
>> +                     crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>> +                     crtc->cpu_transcoder = TRANSCODER_EDP;
>> +
>> +                     DRM_DEBUG_KMS("Pipe %c using transcoder EDP\n",
>> +                                   pipe_name(pipe));
>> +             }
>> +     }
>> +
>>       for_each_pipe(pipe) {
>>               crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>>
>> --
>> 1.7.11.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Paulo Zanoni