[PATCH] sna: CustomEDID fix

[PATCH] sna: CustomEDID fix

[dom.constant]

Hello, 

For my HTPC setup, I'm using the option "CustomEDID".
With this option, output attaching and destroying events leads to crashes.

The following sequence leads to a crash:
- In xorg.conf: Option "CustomEDID" "HDMI2:/etc/my_edid.bin"
- Starting Xorg
- Connect HDMI2
- Disconnect HDMI2
- Reconnect HDMI2
  -> Crash


The crash happens in xf86OutputSetEDID (xorg/xserver/hw/xfree86/modes/xf86Crtc.c) 
at "free(output->MonInfo)". MonInfo is assigned with sna_output->fake_edid_mon 
which is allocated by intel driver in sna_output_load_fake_edid (src/sna/sna_display.c). 


Sequence details:
- Starting Xorg
   -> fake_edid_mon is initialized

- Connect HDMI2
   -> xf86OutputSetEDID is called: 
       - MonInfo is NULL
       - MonInfo is assigned with fake_edid_mon pointer
       - MonInfo is read by Xorg

- Disconnect HDMI2

- Reconnect HDMI2
   -> xf86OutputSetEDID is called:
       - MonInfo is freed thus also fake_edid_mon
       - MonInfo is assigned with fake_edid_mon
       - MonInfo is read but it was freed -> CRASH


The fix consists of a new instance of xf86MonPtr for each calls of xf86OutputSetEDID. 
This instance is initialized with fake_edid_raw which render fake_edid_mon useless. 
With this proposal, the behaviour of an EDID override is similar to a "real" EDID. 

Regards,


Signed-off-by: Dominique Constant <dom.constant@free.fr>
To: Chris Wilson <chris at chris-wilson.co.uk>

diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index ea2f148d..ad805c2f 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -263,7 +263,6 @@ struct sna_output {
         uint32_t edid_blob_id;
         uint32_t edid_len;
         void *edid_raw;
-        xf86MonPtr fake_edid_mon;
         void *fake_edid_raw;
 
         bool has_panel_limits;
@@ -4102,13 +4101,21 @@ static DisplayModePtr
 sna_output_override_edid(xf86OutputPtr output)
 {
         struct sna_output *sna_output = output->driver_private;
+        xf86MonPtr mon = NULL;
+
+        if (sna_output->fake_edid_raw == NULL)
+                return NULL;
 
-        if (sna_output->fake_edid_mon == NULL)
+        mon = xf86InterpretEDID(output->scrn->scrnIndex, sna_output->fake_edid_raw);
+        if (mon == NULL) {
                 return NULL;
+        }
+
+        mon->flags |= MONITOR_EDID_COMPLETE_RAWDATA;
+
+        xf86OutputSetEDID(output, mon);
 
-        xf86OutputSetEDID(output, sna_output->fake_edid_mon);
-        return xf86DDCGetModes(output->scrn->scrnIndex,
-                               sna_output->fake_edid_mon);
+        return xf86DDCGetModes(output->scrn->scrnIndex, mon);
 }
 
 static DisplayModePtr
@@ -4896,7 +4903,6 @@ sna_output_load_fake_edid(xf86OutputPtr output)
         FILE *file;
         void *raw;
         int size;
-        xf86MonPtr mon;
 
         filename = fake_edid_name(output);
         if (filename == NULL)
@@ -4928,16 +4934,6 @@ sna_output_load_fake_edid(xf86OutputPtr output)
         }
         fclose(file);
 
-        mon = xf86InterpretEDID(output->scrn->scrnIndex, raw);
-        if (mon == NULL) {
-                free(raw);
-                goto err;
-        }
-
-        if (mon && size > 128)
-                mon->flags |= MONITOR_EDID_COMPLETE_RAWDATA;
-
-        sna_output->fake_edid_mon = mon;
         sna_output->fake_edid_raw = raw;
 
         xf86DrvMsg(output->scrn->scrnIndex, X_CONFIG,

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] sna: CustomEDID fix

[Jani Nikula]

On Fri, 02 Feb 2018, dom.constant@free.fr wrote:
> For my HTPC setup, I'm using the option "CustomEDID".
> With this option, output attaching and destroying events leads to crashes.
>
> The following sequence leads to a crash:
> - In xorg.conf: Option "CustomEDID" "HDMI2:/etc/my_edid.bin"

I know nothing about xorg customedid stuff.

But there's drm.edid_firmware=HDMI-2:/etc/my_edid.bin (or
drm_kms_helper.edid_firmware on older kernels) module parameter for the
kernel that does this transparently across the stack.

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

Re: [PATCH] sna: CustomEDID fix

[Chris Wilson]

Quoting Jani Nikula (2018-02-02 19:13:53)
> On Fri, 02 Feb 2018, dom.constant@free.fr wrote:
> > For my HTPC setup, I'm using the option "CustomEDID".
> > With this option, output attaching and destroying events leads to crashes.
> >
> > The following sequence leads to a crash:
> > - In xorg.conf: Option "CustomEDID" "HDMI2:/etc/my_edid.bin"
> 
> I know nothing about xorg customedid stuff.
> 
> But there's drm.edid_firmware=HDMI-2:/etc/my_edid.bin (or
> drm_kms_helper.edid_firmware on older kernels) module parameter for the
> kernel that does this transparently across the stack.

There's also the difference in that Xorg only uses the custom EDID to
conveniently load a set of modelines, but the kernel EDID override
describes the sink completely (e.g. audio modes, link protocols, pixel
formats etc).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] sna: CustomEDID fix

[Chris Wilson]

Quoting dom.constant@free.fr (2018-02-02 18:37:12)
> Hello, 
> 
> For my HTPC setup, I'm using the option "CustomEDID".
> With this option, output attaching and destroying events leads to crashes.
> 
> The following sequence leads to a crash:
> - In xorg.conf: Option "CustomEDID" "HDMI2:/etc/my_edid.bin"
> - Starting Xorg
> - Connect HDMI2
> - Disconnect HDMI2
> - Reconnect HDMI2
>   -> Crash
> 
> 
> The crash happens in xf86OutputSetEDID (xorg/xserver/hw/xfree86/modes/xf86Crtc.c) 
> at "free(output->MonInfo)". MonInfo is assigned with sna_output->fake_edid_mon 
> which is allocated by intel driver in sna_output_load_fake_edid (src/sna/sna_display.c). 

Ho hum, looks like I never tested it more than a quick start of Xorg.
Sorry about that :(
 
> Sequence details:
> - Starting Xorg
>    -> fake_edid_mon is initialized
> 
> - Connect HDMI2
>    -> xf86OutputSetEDID is called: 
>        - MonInfo is NULL
>        - MonInfo is assigned with fake_edid_mon pointer
>        - MonInfo is read by Xorg
> 
> - Disconnect HDMI2
> 
> - Reconnect HDMI2
>    -> xf86OutputSetEDID is called:
>        - MonInfo is freed thus also fake_edid_mon
>        - MonInfo is assigned with fake_edid_mon
>        - MonInfo is read but it was freed -> CRASH
> 
> 
> The fix consists of a new instance of xf86MonPtr for each calls of xf86OutputSetEDID. 
> This instance is initialized with fake_edid_raw which render fake_edid_mon useless. 
> With this proposal, the behaviour of an EDID override is similar to a "real" EDID. 
> 
> Regards,
> 
> 
> Signed-off-by: Dominique Constant <dom.constant@free.fr>
> To: Chris Wilson <chris at chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] sna: CustomEDID fix

[Chris Wilson]

Quoting dom.constant@free.fr (2018-02-02 18:37:12)
> Hello, 
> 
> For my HTPC setup, I'm using the option "CustomEDID".
> With this option, output attaching and destroying events leads to crashes.
> 
> The following sequence leads to a crash:
> - In xorg.conf: Option "CustomEDID" "HDMI2:/etc/my_edid.bin"
> - Starting Xorg
> - Connect HDMI2
> - Disconnect HDMI2
> - Reconnect HDMI2
>   -> Crash
> 
> 
> The crash happens in xf86OutputSetEDID (xorg/xserver/hw/xfree86/modes/xf86Crtc.c) 
> at "free(output->MonInfo)". MonInfo is assigned with sna_output->fake_edid_mon 
> which is allocated by intel driver in sna_output_load_fake_edid (src/sna/sna_display.c). 
> 
> 
> Sequence details:
> - Starting Xorg
>    -> fake_edid_mon is initialized
> 
> - Connect HDMI2
>    -> xf86OutputSetEDID is called: 
>        - MonInfo is NULL
>        - MonInfo is assigned with fake_edid_mon pointer
>        - MonInfo is read by Xorg
> 
> - Disconnect HDMI2
> 
> - Reconnect HDMI2
>    -> xf86OutputSetEDID is called:
>        - MonInfo is freed thus also fake_edid_mon
>        - MonInfo is assigned with fake_edid_mon
>        - MonInfo is read but it was freed -> CRASH
> 
> 
> The fix consists of a new instance of xf86MonPtr for each calls of xf86OutputSetEDID. 
> This instance is initialized with fake_edid_raw which render fake_edid_mon useless. 
> With this proposal, the behaviour of an EDID override is similar to a "real" EDID. 
> 
> Regards,
> 
> 
> Signed-off-by: Dominique Constant <dom.constant@free.fr>
> To: Chris Wilson <chris at chris-wilson.co.uk>

Apologies if this is a resend, but could you send me this patch using
"git send-email" (or attach the output of "git format-patch"). git am
is not happy with it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] sna: CustomEDID fix

[dom.constant]

> Quoting dom.constant@free.fr (2018-02-02 18:37:12)
> > Hello,
> > 
> > For my HTPC setup, I'm using the option "CustomEDID".
> > With this option, output attaching and destroying events leads to
> > crashes.
> > 
> > The following sequence leads to a crash:
> > - In xorg.conf: Option "CustomEDID" "HDMI2:/etc/my_edid.bin"
> > - Starting Xorg
> > - Connect HDMI2
> > - Disconnect HDMI2
> > - Reconnect HDMI2
> >   -> Crash
> > 
> > 
> > The crash happens in xf86OutputSetEDID
> > (xorg/xserver/hw/xfree86/modes/xf86Crtc.c)
> > at "free(output->MonInfo)". MonInfo is assigned with
> > sna_output->fake_edid_mon
> > which is allocated by intel driver in sna_output_load_fake_edid
> > (src/sna/sna_display.c).
> > 
> > 
> > Sequence details:
> > - Starting Xorg
> >    -> fake_edid_mon is initialized
> > 
> > - Connect HDMI2
> >    -> xf86OutputSetEDID is called:
> >        - MonInfo is NULL
> >        - MonInfo is assigned with fake_edid_mon pointer
> >        - MonInfo is read by Xorg
> > 
> > - Disconnect HDMI2
> > 
> > - Reconnect HDMI2
> >    -> xf86OutputSetEDID is called:
> >        - MonInfo is freed thus also fake_edid_mon
> >        - MonInfo is assigned with fake_edid_mon
> >        - MonInfo is read but it was freed -> CRASH
> > 
> > 
> > The fix consists of a new instance of xf86MonPtr for each calls of
> > xf86OutputSetEDID.
> > This instance is initialized with fake_edid_raw which render
> > fake_edid_mon useless.
> > With this proposal, the behaviour of an EDID override is similar to
> > a "real" EDID.
> > 
> > Regards,
> > 
> > 
> > Signed-off-by: Dominique Constant <dom.constant@free.fr>
> > To: Chris Wilson <chris at chris-wilson.co.uk>
> 
> Apologies if this is a resend, but could you send me this patch using
> "git send-email" (or attach the output of "git format-patch"). git am
> is not happy with it.
> -Chris
> 


Hello,
Thank you for taking into consideration this patch.

Regards,

Dominique CONSTANT

Signed-off-by: Dominique Constant <dom.constant@free.fr>
---
 src/sna/sna_display.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index ea2f148d..ad805c2f 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -263,7 +263,6 @@ struct sna_output {
 	uint32_t edid_blob_id;
 	uint32_t edid_len;
 	void *edid_raw;
-	xf86MonPtr fake_edid_mon;
 	void *fake_edid_raw;
 
 	bool has_panel_limits;
@@ -4102,13 +4101,21 @@ static DisplayModePtr
 sna_output_override_edid(xf86OutputPtr output)
 {
 	struct sna_output *sna_output = output->driver_private;
+	xf86MonPtr mon = NULL;
+
+	if (sna_output->fake_edid_raw == NULL)
+		return NULL;
 
-	if (sna_output->fake_edid_mon == NULL)
+	mon = xf86InterpretEDID(output->scrn->scrnIndex, sna_output->fake_edid_raw);
+	if (mon == NULL) {
 		return NULL;
+	}
+
+	mon->flags |= MONITOR_EDID_COMPLETE_RAWDATA;
+
+	xf86OutputSetEDID(output, mon);
 
-	xf86OutputSetEDID(output, sna_output->fake_edid_mon);
-	return xf86DDCGetModes(output->scrn->scrnIndex,
-			       sna_output->fake_edid_mon);
+	return xf86DDCGetModes(output->scrn->scrnIndex, mon);
 }
 
 static DisplayModePtr
@@ -4896,7 +4903,6 @@ sna_output_load_fake_edid(xf86OutputPtr output)
 	FILE *file;
 	void *raw;
 	int size;
-	xf86MonPtr mon;
 
 	filename = fake_edid_name(output);
 	if (filename == NULL)
@@ -4928,16 +4934,6 @@ sna_output_load_fake_edid(xf86OutputPtr output)
 	}
 	fclose(file);
 
-	mon = xf86InterpretEDID(output->scrn->scrnIndex, raw);
-	if (mon == NULL) {
-		free(raw);
-		goto err;
-	}
-
-	if (mon && size > 128)
-		mon->flags |= MONITOR_EDID_COMPLETE_RAWDATA;
-
-	sna_output->fake_edid_mon = mon;
 	sna_output->fake_edid_raw = raw;
 
 	xf86DrvMsg(output->scrn->scrnIndex, X_CONFIG,
-- 
2.15.1


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] sna: CustomEDID fix

[Chris Wilson]

Quoting dom.constant@free.fr (2018-02-06 18:57:05)
> 
> 
> > Quoting dom.constant@free.fr (2018-02-02 18:37:12)
> > > Hello,
> > > 
> > > For my HTPC setup, I'm using the option "CustomEDID".
> > > With this option, output attaching and destroying events leads to
> > > crashes.
> > > 
> > > The following sequence leads to a crash:
> > > - In xorg.conf: Option "CustomEDID" "HDMI2:/etc/my_edid.bin"
> > > - Starting Xorg
> > > - Connect HDMI2
> > > - Disconnect HDMI2
> > > - Reconnect HDMI2
> > >   -> Crash
> > > 
> > > 
> > > The crash happens in xf86OutputSetEDID
> > > (xorg/xserver/hw/xfree86/modes/xf86Crtc.c)
> > > at "free(output->MonInfo)". MonInfo is assigned with
> > > sna_output->fake_edid_mon
> > > which is allocated by intel driver in sna_output_load_fake_edid
> > > (src/sna/sna_display.c).
> > > 
> > > 
> > > Sequence details:
> > > - Starting Xorg
> > >    -> fake_edid_mon is initialized
> > > 
> > > - Connect HDMI2
> > >    -> xf86OutputSetEDID is called:
> > >        - MonInfo is NULL
> > >        - MonInfo is assigned with fake_edid_mon pointer
> > >        - MonInfo is read by Xorg
> > > 
> > > - Disconnect HDMI2
> > > 
> > > - Reconnect HDMI2
> > >    -> xf86OutputSetEDID is called:
> > >        - MonInfo is freed thus also fake_edid_mon
> > >        - MonInfo is assigned with fake_edid_mon
> > >        - MonInfo is read but it was freed -> CRASH
> > > 
> > > 
> > > The fix consists of a new instance of xf86MonPtr for each calls of
> > > xf86OutputSetEDID.
> > > This instance is initialized with fake_edid_raw which render
> > > fake_edid_mon useless.
> > > With this proposal, the behaviour of an EDID override is similar to
> > > a "real" EDID.
> > > 
> > > Regards,
> > > 
> > > 
> > > Signed-off-by: Dominique Constant <dom.constant@free.fr>
> > > To: Chris Wilson <chris at chris-wilson.co.uk>
> > 
> > Apologies if this is a resend, but could you send me this patch using
> > "git send-email" (or attach the output of "git format-patch"). git am
> > is not happy with it.
> > -Chris
> > 
> 
> 
> Hello,
> Thank you for taking into consideration this patch.

Finally, got around to picking up patches for -intel. Thank you very
much for the fix,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx