[0/3] drm/i915/display: handle hdmi connector init failures

Message

Jani Nikula Nov. 14, 2024, 3:52 p.m. UTC

This is an attempt to resolve [1].

BR,
Jani.


[1] https://lore.kernel.org/r/[e-mail]


Cc: Sergey Senozhatsky <[e-mail]>
Cc: Ville Syrjala <[e-mail]>

Jani Nikula (3):
  drm/i915/hdmi: propagate errors from intel_hdmi_init_connector()
  drm/i915/hdmi: add error handling in g4x_hdmi_init()
  drm/i915/ddi: propagate errors from intel_ddi_init_hdmi_connector()

 drivers/gpu/drm/i915/display/g4x_hdmi.c   | 35 +++++++++++++++--------
 drivers/gpu/drm/i915/display/g4x_hdmi.h   |  5 ++--
 drivers/gpu/drm/i915/display/intel_ddi.c  |  6 +++-
 drivers/gpu/drm/i915/display/intel_hdmi.c | 10 ++++---
 drivers/gpu/drm/i915/display/intel_hdmi.h |  4 +--
 5 files changed, 39 insertions(+), 21 deletions(-)

Comments

Sergey Senozhatsky Nov. 15, 2024, 5 a.m. UTC | #1

On (24/11/14 17:52), Jani Nikula wrote:
> This is an attempt to resolve [1].
> 
> [1] https://lore.kernel.org/r/[e-mail]
> 
> Jani Nikula (3):
>   drm/i915/hdmi: propagate errors from intel_hdmi_init_connector()
>   drm/i915/hdmi: add error handling in g4x_hdmi_init()
>   drm/i915/ddi: propagate errors from intel_ddi_init_hdmi_connector()

So these look reasonable and good to me, and I think landing them
makes a lot of sense.  For that part feel free to add:

Reported-and-tested-by: Sergey Senozhatsky <[e-mail]>
and FWIW
Reviewed-by: Sergey Senozhatsky <[e-mail]>



Now, regarding the issue in question - we are getting there, but not
quite there yet.

I'm still seeing multiple NULL-ptr derefs, namely on

 DDI A/PHY A
 DDI TC1/PHY TC1
 DDI TC2/PHY TC2
 DDI TC3/PHY TC3
 DDI TC4/PHY TC4

The thing is, all of them are DP connectors, we only ever call
intel_ddi_init_dp_connector() on them, so dig_port->hdmi
is not init-ed, wheres intel_hdmi_encoder_shutdown() doesn't
distinguish and treats everyone as HDMI connector, dereferencing
->hdmi.

With the small addition I don't see null-ptr derefs anymore:

---

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 1abf080b4a20..0c6bb378b708 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2377,6 +2377,11 @@ void intel_hdmi_encoder_shutdown(struct intel_encoder *encoder)
 {
        struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 
+       if (intel_encoder_is_dp(encoder)) {
+               pr_err(":: oops another day ::\n");
+               return;
+       }
+
        /*
         * Give a hand to buggy BIOSen which forget to turn
         * the TMDS output buffers back on after a reboot.

Sergey Senozhatsky Dec. 3, 2024, 12:31 p.m. UTC | #2

On (24/11/15 14:00), Sergey Senozhatsky wrote:
> On (24/11/14 17:52), Jani Nikula wrote:
> > This is an attempt to resolve [1].
> > 
> > [1] https://lore.kernel.org/r/[e-mail]
> > 
> > Jani Nikula (3):
> >   drm/i915/hdmi: propagate errors from intel_hdmi_init_connector()
> >   drm/i915/hdmi: add error handling in g4x_hdmi_init()
> >   drm/i915/ddi: propagate errors from intel_ddi_init_hdmi_connector()
> 
> So these look reasonable and good to me, and I think landing them
> makes a lot of sense.  For that part feel free to add:
> 
> Reported-and-tested-by: Sergey Senozhatsky <[e-mail]>
> and FWIW
> Reviewed-by: Sergey Senozhatsky <[e-mail]>
> 
> 
> 
> Now, regarding the issue in question - we are getting there, but not
> quite there yet.
> 
> I'm still seeing multiple NULL-ptr derefs, namely on
> 
>  DDI A/PHY A
>  DDI TC1/PHY TC1
>  DDI TC2/PHY TC2
>  DDI TC3/PHY TC3
>  DDI TC4/PHY TC4
> 
> The thing is, all of them are DP connectors, we only ever call
> intel_ddi_init_dp_connector() on them, so dig_port->hdmi
> is not init-ed, wheres intel_hdmi_encoder_shutdown() doesn't
> distinguish and treats everyone as HDMI connector, dereferencing
> ->hdmi.
> 
> With the small addition I don't see null-ptr derefs anymore:

Gentle ping on this.

Jani Nikula Dec. 4, 2024, 3:08 p.m. UTC | #3

On Tue, 03 Dec 2024, Sergey Senozhatsky <[e-mail]> wrote:
> On (24/11/15 14:00), Sergey Senozhatsky wrote:
>> On (24/11/14 17:52), Jani Nikula wrote:
>> > This is an attempt to resolve [1].
>> > 
>> > [1] https://lore.kernel.org/r/[e-mail]
>> > 
>> > Jani Nikula (3):
>> >   drm/i915/hdmi: propagate errors from intel_hdmi_init_connector()
>> >   drm/i915/hdmi: add error handling in g4x_hdmi_init()
>> >   drm/i915/ddi: propagate errors from intel_ddi_init_hdmi_connector()
>> 
>> So these look reasonable and good to me, and I think landing them
>> makes a lot of sense.  For that part feel free to add:
>> 
>> Reported-and-tested-by: Sergey Senozhatsky <[e-mail]>
>> and FWIW
>> Reviewed-by: Sergey Senozhatsky <[e-mail]>
>> 
>> 
>> 
>> Now, regarding the issue in question - we are getting there, but not
>> quite there yet.
>> 
>> I'm still seeing multiple NULL-ptr derefs, namely on
>> 
>>  DDI A/PHY A
>>  DDI TC1/PHY TC1
>>  DDI TC2/PHY TC2
>>  DDI TC3/PHY TC3
>>  DDI TC4/PHY TC4
>> 
>> The thing is, all of them are DP connectors, we only ever call
>> intel_ddi_init_dp_connector() on them, so dig_port->hdmi
>> is not init-ed, wheres intel_hdmi_encoder_shutdown() doesn't
>> distinguish and treats everyone as HDMI connector, dereferencing
>> ->hdmi.
>> 
>> With the small addition I don't see null-ptr derefs anymore:
>
> Gentle ping on this.

v2 with two additional patches at [1].

BR,
Jani.

[1] https://lore.kernel.org/r/[e-mail]