[1/4] drm/i915: read dpcd 0 - 12 & link_status always
diff mbox

Message ID 1440665312-26698-2-git-send-email-sivakumar.thulasimani@intel.com
State New
Headers show

Commit Message

Sivakumar Thulasimani Aug. 27, 2015, 8:48 a.m. UTC
From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

Compliance requires the driver to read dpcd register 0 to 12 and
registers 0x200 to 0x205 to be read always.
Current code performs dpcd read for short pulse interrupts only
if the sink is enabled. This patch forces read for link status
and registers 0 to 12.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Jani Nikula Sept. 1, 2015, 10:16 a.m. UTC | #1
On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> Compliance requires the driver to read dpcd register 0 to 12 and
> registers 0x200 to 0x205 to be read always.
> Current code performs dpcd read for short pulse interrupts only
> if the sink is enabled. This patch forces read for link status
> and registers 0 to 12.

While this seems a bit silly from the driver perspective, I don't see it
doing much harm either.

Note that We do read dpcd 0 to 14 no matter what the spec says.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8a66a44..76561e0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4374,12 +4374,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	if (!intel_encoder->base.crtc)
> -		return;
> -
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> -
>  	/* Try to read receiver status if the link appears to be up */
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		return;
> @@ -4390,6 +4384,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  		return;
>  	}
>  
> +	if (!intel_encoder->base.crtc)
> +		return;
> +
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return;
> +
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>  	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> -- 
> 1.7.9.5
>
Daniel Vetter Sept. 2, 2015, 9:04 a.m. UTC | #2
On Tue, Sep 01, 2015 at 01:16:49PM +0300, Jani Nikula wrote:
> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> > From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
> >
> > Compliance requires the driver to read dpcd register 0 to 12 and
> > registers 0x200 to 0x205 to be read always.
> > Current code performs dpcd read for short pulse interrupts only
> > if the sink is enabled. This patch forces read for link status
> > and registers 0 to 12.
> 
> While this seems a bit silly from the driver perspective, I don't see it
> doing much harm either.

I don't think this is silly, but fixing it like this might be - currently
we don't do _any_ detection of sink ports, so if you have a hub with two
outputs and then plug in another one and plug out the first userspace
won't reprobe. So probably what we should be doing is not just read the
dpcd, but actually look at it, figure out whether something has changed
and make sure we send out the uevent even if the hpd line stays unchanged
on short pulses to make userspace aware of the changes.

Punting on this for now since I suspect there's a bigger story to be had
here ...
-Daniel

> 
> Note that We do read dpcd 0 to 14 no matter what the spec says.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> >
> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |   12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 8a66a44..76561e0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4374,12 +4374,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> >  
> >  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >  
> > -	if (!intel_encoder->base.crtc)
> > -		return;
> > -
> > -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > -		return;
> > -
> >  	/* Try to read receiver status if the link appears to be up */
> >  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >  		return;
> > @@ -4390,6 +4384,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> >  		return;
> >  	}
> >  
> > +	if (!intel_encoder->base.crtc)
> > +		return;
> > +
> > +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > +		return;
> > +
> >  	/* Try to read the source of the interrupt */
> >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> >  	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> > -- 
> > 1.7.9.5
> >
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Sept. 3, 2015, 12:25 p.m. UTC | #3
On Wed, 02 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Sep 01, 2015 at 01:16:49PM +0300, Jani Nikula wrote:
>> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> > From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>> >
>> > Compliance requires the driver to read dpcd register 0 to 12 and
>> > registers 0x200 to 0x205 to be read always.
>> > Current code performs dpcd read for short pulse interrupts only
>> > if the sink is enabled. This patch forces read for link status
>> > and registers 0 to 12.
>> 
>> While this seems a bit silly from the driver perspective, I don't see it
>> doing much harm either.
>
> I don't think this is silly, but fixing it like this might be - currently
> we don't do _any_ detection of sink ports, so if you have a hub with two
> outputs and then plug in another one and plug out the first userspace
> won't reprobe. So probably what we should be doing is not just read the
> dpcd, but actually look at it, figure out whether something has changed
> and make sure we send out the uevent even if the hpd line stays unchanged
> on short pulses to make userspace aware of the changes.
>
> Punting on this for now since I suspect there's a bigger story to be had
> here ...

Well, I'll bet this would be a preliminary step for that anyway.

BR,
Jani.


> -Daniel
>
>> 
>> Note that We do read dpcd 0 to 14 no matter what the spec says.
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> >
>> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c |   12 ++++++------
>> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 8a66a44..76561e0 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -4374,12 +4374,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>> >  
>> >  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>> >  
>> > -	if (!intel_encoder->base.crtc)
>> > -		return;
>> > -
>> > -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> > -		return;
>> > -
>> >  	/* Try to read receiver status if the link appears to be up */
>> >  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> >  		return;
>> > @@ -4390,6 +4384,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>> >  		return;
>> >  	}
>> >  
>> > +	if (!intel_encoder->base.crtc)
>> > +		return;
>> > +
>> > +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> > +		return;
>> > +
>> >  	/* Try to read the source of the interrupt */
>> >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>> >  	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>> > -- 
>> > 1.7.9.5
>> >
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Sept. 4, 2015, 7:46 a.m. UTC | #4
On Thu, Sep 03, 2015 at 03:25:04PM +0300, Jani Nikula wrote:
> On Wed, 02 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Sep 01, 2015 at 01:16:49PM +0300, Jani Nikula wrote:
> >> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> >> > From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
> >> >
> >> > Compliance requires the driver to read dpcd register 0 to 12 and
> >> > registers 0x200 to 0x205 to be read always.
> >> > Current code performs dpcd read for short pulse interrupts only
> >> > if the sink is enabled. This patch forces read for link status
> >> > and registers 0 to 12.
> >> 
> >> While this seems a bit silly from the driver perspective, I don't see it
> >> doing much harm either.
> >
> > I don't think this is silly, but fixing it like this might be - currently
> > we don't do _any_ detection of sink ports, so if you have a hub with two
> > outputs and then plug in another one and plug out the first userspace
> > won't reprobe. So probably what we should be doing is not just read the
> > dpcd, but actually look at it, figure out whether something has changed
> > and make sure we send out the uevent even if the hpd line stays unchanged
> > on short pulses to make userspace aware of the changes.
> >
> > Punting on this for now since I suspect there's a bigger story to be had
> > here ...
> 
> Well, I'll bet this would be a preliminary step for that anyway.

Then the commit message should explain that. Adding code that's not needed
just for compliance to be happy just feels very fishy. And if we have more
work to do won't really gain us much since for the full monty we probably
need to juggle a few pieces in the overall picture.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8a66a44..76561e0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4374,12 +4374,6 @@  intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->base.crtc)
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		return;
@@ -4390,6 +4384,12 @@  intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {