[1/3] drm/i915: Dont enable hpd for eDP
diff mbox

Message ID 1439183136-23798-2-git-send-email-sonika.jindal@intel.com
State New
Headers show

Commit Message

sonika.jindal@intel.com Aug. 10, 2015, 5:05 a.m. UTC
With HPD support added for all ports including PORT_A, setting hpd_pin will
result in enabling of hpd to edp as well. There is no need to enable HPD on
PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
be connected. it can be added back when required

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sivakumar Thulasimani Aug. 10, 2015, 5:24 a.m. UTC | #1
Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>

On 8/10/2015 10:35 AM, Sonika Jindal wrote:
> With HPD support added for all ports including PORT_A, setting hpd_pin will
> result in enabling of hpd to edp as well. There is no need to enable HPD on
> PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
> be connected. it can be added back when required
>
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fcc64e5..5a614c9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5784,7 +5784,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>   	/* Set up the hotplug pin. */
>   	switch (port) {
>   	case PORT_A:
> -		intel_encoder->hpd_pin = HPD_PORT_A;
> +		/* Not enabling edp interrupts */
>   		break;
>   	case PORT_B:
>   		intel_encoder->hpd_pin = HPD_PORT_B;
Jani Nikula Aug. 10, 2015, 11:37 a.m. UTC | #2
On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>
> On 8/10/2015 10:35 AM, Sonika Jindal wrote:
>> With HPD support added for all ports including PORT_A, setting hpd_pin will
>> result in enabling of hpd to edp as well. There is no need to enable HPD on
>> PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
>> be connected. it can be added back when required

What? You can't just go ahead and remove HPD from eDP sinks.

BR,
Jani.


>>
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index fcc64e5..5a614c9 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -5784,7 +5784,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>   	/* Set up the hotplug pin. */
>>   	switch (port) {
>>   	case PORT_A:
>> -		intel_encoder->hpd_pin = HPD_PORT_A;
>> +		/* Not enabling edp interrupts */
>>   		break;
>>   	case PORT_B:
>>   		intel_encoder->hpd_pin = HPD_PORT_B;
>
> -- 
> regards,
> Sivakumar
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sivakumar Thulasimani Aug. 10, 2015, 11:38 a.m. UTC | #3
On 8/10/2015 5:07 PM, Jani Nikula wrote:
> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>
>> On 8/10/2015 10:35 AM, Sonika Jindal wrote:
>>> With HPD support added for all ports including PORT_A, setting hpd_pin will
>>> result in enabling of hpd to edp as well. There is no need to enable HPD on
>>> PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
>>> be connected. it can be added back when required
> What? You can't just go ahead and remove HPD from eDP sinks.
>
> BR,
> Jani.
Nope, we are not removing HPD for edp sinks, it was never there in the 
first place. It was
enabled for CHV (even there by mistake since PORT B/C was both DP and 
eDP) but it was
never there for any other plaforms nor is it used for any purpose (PSR 
must use it, but i
dont see code for it as well).
>
>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_dp.c |    2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index fcc64e5..5a614c9 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -5784,7 +5784,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>>    	/* Set up the hotplug pin. */
>>>    	switch (port) {
>>>    	case PORT_A:
>>> -		intel_encoder->hpd_pin = HPD_PORT_A;
>>> +		/* Not enabling edp interrupts */
>>>    		break;
>>>    	case PORT_B:
>>>    		intel_encoder->hpd_pin = HPD_PORT_B;
>> -- 
>> regards,
>> Sivakumar
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Aug. 10, 2015, 12:14 p.m. UTC | #4
On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> On 8/10/2015 5:07 PM, Jani Nikula wrote:
>> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>>> Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>
>>> On 8/10/2015 10:35 AM, Sonika Jindal wrote:
>>>> With HPD support added for all ports including PORT_A, setting hpd_pin will
>>>> result in enabling of hpd to edp as well. There is no need to enable HPD on
>>>> PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
>>>> be connected. it can be added back when required
>> What? You can't just go ahead and remove HPD from eDP sinks.
>>
>> BR,
>> Jani.
> Nope, we are not removing HPD for edp sinks, it was never there in the 
> first place. It was
> enabled for CHV (even there by mistake since PORT B/C was both DP and 
> eDP) but it was
> never there for any other plaforms nor is it used for any purpose (PSR 
> must use it, but i
> dont see code for it as well).

Are you saying there's no HPD enabled in our *hardware* for eDP? Or
driver?

My point is, is this patch making it harder to enable eDP hpd handling
(e.g. for PSR or DP link re-training) in the future? We currently take
it into account in a few places, and if we start removing that, it will
be a loss of effort to first remove and then add it back.

BR,
Jani.



>>
>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c |    2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index fcc64e5..5a614c9 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -5784,7 +5784,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>>>    	/* Set up the hotplug pin. */
>>>>    	switch (port) {
>>>>    	case PORT_A:
>>>> -		intel_encoder->hpd_pin = HPD_PORT_A;
>>>> +		/* Not enabling edp interrupts */
>>>>    		break;
>>>>    	case PORT_B:
>>>>    		intel_encoder->hpd_pin = HPD_PORT_B;
>>> -- 
>>> regards,
>>> Sivakumar
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> regards,
> Sivakumar
>
Ville Syrjälä Aug. 10, 2015, 12:16 p.m. UTC | #5
On Mon, Aug 10, 2015 at 03:14:36PM +0300, Jani Nikula wrote:
> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> > On 8/10/2015 5:07 PM, Jani Nikula wrote:
> >> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> >>> Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> >>>
> >>> On 8/10/2015 10:35 AM, Sonika Jindal wrote:
> >>>> With HPD support added for all ports including PORT_A, setting hpd_pin will
> >>>> result in enabling of hpd to edp as well. There is no need to enable HPD on
> >>>> PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
> >>>> be connected. it can be added back when required
> >> What? You can't just go ahead and remove HPD from eDP sinks.
> >>
> >> BR,
> >> Jani.
> > Nope, we are not removing HPD for edp sinks, it was never there in the 
> > first place. It was
> > enabled for CHV (even there by mistake since PORT B/C was both DP and 
> > eDP) but it was
> > never there for any other plaforms nor is it used for any purpose (PSR 
> > must use it, but i
> > dont see code for it as well).
> 
> Are you saying there's no HPD enabled in our *hardware* for eDP? Or
> driver?
> 
> My point is, is this patch making it harder to enable eDP hpd handling
> (e.g. for PSR or DP link re-training) in the future? We currently take
> it into account in a few places, and if we start removing that, it will
> be a loss of effort to first remove and then add it back.

I have a branch with (untested) port A/E HPD support (+ a bunch of SPT+
irq handler reorganization). I guess I should post that in case people
are interested in this stuff.
Sivakumar Thulasimani Aug. 10, 2015, 12:21 p.m. UTC | #6
On 8/10/2015 5:44 PM, Jani Nikula wrote:
> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> On 8/10/2015 5:07 PM, Jani Nikula wrote:
>>> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>>>> Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>>
>>>> On 8/10/2015 10:35 AM, Sonika Jindal wrote:
>>>>> With HPD support added for all ports including PORT_A, setting hpd_pin will
>>>>> result in enabling of hpd to edp as well. There is no need to enable HPD on
>>>>> PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
>>>>> be connected. it can be added back when required
>>> What? You can't just go ahead and remove HPD from eDP sinks.
>>>
>>> BR,
>>> Jani.
>> Nope, we are not removing HPD for edp sinks, it was never there in the
>> first place. It was
>> enabled for CHV (even there by mistake since PORT B/C was both DP and
>> eDP) but it was
>> never there for any other plaforms nor is it used for any purpose (PSR
>> must use it, but i
>> dont see code for it as well).
> Are you saying there's no HPD enabled in our *hardware* for eDP? Or
> driver?
>
> My point is, is this patch making it harder to enable eDP hpd handling
> (e.g. for PSR or DP link re-training) in the future? We currently take
> it into account in a few places, and if we start removing that, it will
> be a loss of effort to first remove and then add it back.
>
> BR,
> Jani.
i was referring to our driver only.

Our VLV/CHV code already receives hpd for every pps on and off which is
later ignored. if we dont disable HPD on eDPs this behavior will be extended
for all platforms which i feel is too costly to keep enabled when there 
is no
purpose for it right now.

if anyone later requires HPD , they might need it for PSR panels only and
since that code is not available as of now, this can be added along with 
those
changes so i feel this is the better tradeoff :)
>
>
>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/intel_dp.c |    2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index fcc64e5..5a614c9 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -5784,7 +5784,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>>>>     	/* Set up the hotplug pin. */
>>>>>     	switch (port) {
>>>>>     	case PORT_A:
>>>>> -		intel_encoder->hpd_pin = HPD_PORT_A;
>>>>> +		/* Not enabling edp interrupts */
>>>>>     		break;
>>>>>     	case PORT_B:
>>>>>     		intel_encoder->hpd_pin = HPD_PORT_B;
>>>> -- 
>>>> regards,
>>>> Sivakumar
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> regards,
>> Sivakumar
>>
Daniel Vetter Aug. 12, 2015, 12:56 p.m. UTC | #7
On Mon, Aug 10, 2015 at 05:51:48PM +0530, Sivakumar Thulasimani wrote:
> 
> 
> On 8/10/2015 5:44 PM, Jani Nikula wrote:
> >On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> >>On 8/10/2015 5:07 PM, Jani Nikula wrote:
> >>>On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> >>>>Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> >>>>
> >>>>On 8/10/2015 10:35 AM, Sonika Jindal wrote:
> >>>>>With HPD support added for all ports including PORT_A, setting hpd_pin will
> >>>>>result in enabling of hpd to edp as well. There is no need to enable HPD on
> >>>>>PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
> >>>>>be connected. it can be added back when required
> >>>What? You can't just go ahead and remove HPD from eDP sinks.
> >>>
> >>>BR,
> >>>Jani.
> >>Nope, we are not removing HPD for edp sinks, it was never there in the
> >>first place. It was
> >>enabled for CHV (even there by mistake since PORT B/C was both DP and
> >>eDP) but it was
> >>never there for any other plaforms nor is it used for any purpose (PSR
> >>must use it, but i
> >>dont see code for it as well).
> >Are you saying there's no HPD enabled in our *hardware* for eDP? Or
> >driver?
> >
> >My point is, is this patch making it harder to enable eDP hpd handling
> >(e.g. for PSR or DP link re-training) in the future? We currently take
> >it into account in a few places, and if we start removing that, it will
> >be a loss of effort to first remove and then add it back.
> >
> >BR,
> >Jani.
> i was referring to our driver only.
> 
> Our VLV/CHV code already receives hpd for every pps on and off which is
> later ignored. if we dont disable HPD on eDPs this behavior will be extended
> for all platforms which i feel is too costly to keep enabled when there is
> no
> purpose for it right now.

don't optimize code because you "feel it's costly", only do it when you
have hard numbers. One interrupt per pps on or off transition won't be
measurable at all.
-Daniel
Sivakumar Thulasimani Aug. 13, 2015, 4:05 a.m. UTC | #8
On 8/12/2015 6:26 PM, Daniel Vetter wrote:
> On Mon, Aug 10, 2015 at 05:51:48PM +0530, Sivakumar Thulasimani wrote:
>>
>> On 8/10/2015 5:44 PM, Jani Nikula wrote:
>>> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>>>> On 8/10/2015 5:07 PM, Jani Nikula wrote:
>>>>> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>>>>>> Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>>>>
>>>>>> On 8/10/2015 10:35 AM, Sonika Jindal wrote:
>>>>>>> With HPD support added for all ports including PORT_A, setting hpd_pin will
>>>>>>> result in enabling of hpd to edp as well. There is no need to enable HPD on
>>>>>>> PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
>>>>>>> be connected. it can be added back when required
>>>>> What? You can't just go ahead and remove HPD from eDP sinks.
>>>>>
>>>>> BR,
>>>>> Jani.
>>>> Nope, we are not removing HPD for edp sinks, it was never there in the
>>>> first place. It was
>>>> enabled for CHV (even there by mistake since PORT B/C was both DP and
>>>> eDP) but it was
>>>> never there for any other plaforms nor is it used for any purpose (PSR
>>>> must use it, but i
>>>> dont see code for it as well).
>>> Are you saying there's no HPD enabled in our *hardware* for eDP? Or
>>> driver?
>>>
>>> My point is, is this patch making it harder to enable eDP hpd handling
>>> (e.g. for PSR or DP link re-training) in the future? We currently take
>>> it into account in a few places, and if we start removing that, it will
>>> be a loss of effort to first remove and then add it back.
>>>
>>> BR,
>>> Jani.
>> i was referring to our driver only.
>>
>> Our VLV/CHV code already receives hpd for every pps on and off which is
>> later ignored. if we dont disable HPD on eDPs this behavior will be extended
>> for all platforms which i feel is too costly to keep enabled when there is
>> no
>> purpose for it right now.
> don't optimize code because you "feel it's costly", only do it when you
> have hard numbers. One interrupt per pps on or off transition won't be
> measurable at all.
> -Daniel
let me rephrase my concern then :)
a) HPD was never enabled before this patch for edp
b) this patch series will enable hpd for edp
so why should we allow hpd for edp when no one is using it and will 
cause problems
unless ignored explicitly ?
Ville Syrjälä Aug. 13, 2015, 3:07 p.m. UTC | #9
On Thu, Aug 13, 2015 at 09:35:21AM +0530, Sivakumar Thulasimani wrote:
> 
> 
> On 8/12/2015 6:26 PM, Daniel Vetter wrote:
> > On Mon, Aug 10, 2015 at 05:51:48PM +0530, Sivakumar Thulasimani wrote:
> >>
> >> On 8/10/2015 5:44 PM, Jani Nikula wrote:
> >>> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> >>>> On 8/10/2015 5:07 PM, Jani Nikula wrote:
> >>>>> On Mon, 10 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> >>>>>> Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> >>>>>>
> >>>>>> On 8/10/2015 10:35 AM, Sonika Jindal wrote:
> >>>>>>> With HPD support added for all ports including PORT_A, setting hpd_pin will
> >>>>>>> result in enabling of hpd to edp as well. There is no need to enable HPD on
> >>>>>>> PORT_A hence this patch removes hpd_pin update for PORT_A, where edp will
> >>>>>>> be connected. it can be added back when required
> >>>>> What? You can't just go ahead and remove HPD from eDP sinks.
> >>>>>
> >>>>> BR,
> >>>>> Jani.
> >>>> Nope, we are not removing HPD for edp sinks, it was never there in the
> >>>> first place. It was
> >>>> enabled for CHV (even there by mistake since PORT B/C was both DP and
> >>>> eDP) but it was
> >>>> never there for any other plaforms nor is it used for any purpose (PSR
> >>>> must use it, but i
> >>>> dont see code for it as well).
> >>> Are you saying there's no HPD enabled in our *hardware* for eDP? Or
> >>> driver?
> >>>
> >>> My point is, is this patch making it harder to enable eDP hpd handling
> >>> (e.g. for PSR or DP link re-training) in the future? We currently take
> >>> it into account in a few places, and if we start removing that, it will
> >>> be a loss of effort to first remove and then add it back.
> >>>
> >>> BR,
> >>> Jani.
> >> i was referring to our driver only.
> >>
> >> Our VLV/CHV code already receives hpd for every pps on and off which is
> >> later ignored. if we dont disable HPD on eDPs this behavior will be extended
> >> for all platforms which i feel is too costly to keep enabled when there is
> >> no
> >> purpose for it right now.
> > don't optimize code because you "feel it's costly", only do it when you
> > have hard numbers. One interrupt per pps on or off transition won't be
> > measurable at all.
> > -Daniel
> let me rephrase my concern then :)
> a) HPD was never enabled before this patch for edp

It was for port!=A, which means all VLV/CHV + some other machines with
eDP on port D.

> b) this patch series will enable hpd for edp
> so why should we allow hpd for edp when no one is using it and will 
> cause problems
> unless ignored explicitly ?

It's not clear it would cause problems. My ILK still seems happy now that
I enabled port A HPD on it [1]. As are the VLV/CHV machines. On BSW we
had problems but that turned out to be misconfigured pullups on the HPD
pins. Before that got fixed there was a lot of noise on the line
whenever the panel was off, and that spurred me to write the patch
series to keep the HPD disabled while the port is disabled [2].

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-August/073559.html
[2] http://lists.freedesktop.org/archives/intel-gfx/2015-January/058173.html

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fcc64e5..5a614c9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5784,7 +5784,7 @@  intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	/* Set up the hotplug pin. */
 	switch (port) {
 	case PORT_A:
-		intel_encoder->hpd_pin = HPD_PORT_A;
+		/* Not enabling edp interrupts */
 		break;
 	case PORT_B:
 		intel_encoder->hpd_pin = HPD_PORT_B;