Sean Paul writes:
On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich eich@suse.de wrote:
drm_get_edid() returns a pointer to an EDID block. The caller is responsible to free this pointer itself. Here the pointer gets assigned to the local variable raw_edid. Therefore it should be freed before the variable goes out of scope.
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bc87bca 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector, DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), raw_edid->width_cm, raw_edid->height_cm);
kfree(raw_edid);This will actually cause the memory to be freed twice.
The reason this happens is drm_get_edid attaches this to connector->display_info.raw_edid, which is then freed in the exynos_drm_connector function that gets the edid.
No, the raw_edid member is gone from struct drm_display_info. All other drivers free the edid data returned by drm_get_edid() themselves. I would actually prefer if the edid data would be freed by drm_connector_destroy rather than by the drivers as this would allow us to cache an edid without creating copies to pass to the drivers. However some drivers store the pointer to returned edid block somewhere in their own data structures, so if we do this it can easily get out of sync when we reread the edid and it has changed for whatever reason in which case the driver will have a stale and invalid pointer.
The whole thing is ugly, and needs to be revised. I've uploaded a patch to refactor this against the chromium tree, but haven't yet rebased against upstream. See https://gerrit.chromium.org/gerrit/#/c/38406/
For now, please drop this patch.
I've looked at the code in the exynos driver in the drm_next branch - there's nothing that destroys this edid data any more.
Cheers, Egbert.
On Tue, Nov 20, 2012 at 3:29 PM, Egbert Eich eich@suse.com wrote:
Sean Paul writes:
On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich eich@suse.de wrote:
drm_get_edid() returns a pointer to an EDID block. The caller is responsible to free this pointer itself. Here the pointer gets assigned to the local variable raw_edid. Therefore it should be freed before the variable goes out of scope.
Signed-off-by: Egbert Eich eich@suse.de
Reviewed-by: Sean Paul seanpaul@chromium.org
drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bc87bca 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector, DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), raw_edid->width_cm, raw_edid->height_cm);
kfree(raw_edid);This will actually cause the memory to be freed twice.
The reason this happens is drm_get_edid attaches this to connector->display_info.raw_edid, which is then freed in the exynos_drm_connector function that gets the edid.
No, the raw_edid member is gone from struct drm_display_info. All other drivers free the edid data returned by drm_get_edid() themselves.
A ha! I should have checked first, my apologies.
I would actually prefer if the edid data would be freed by drm_connector_destroy rather than by the drivers as this would allow us to cache an edid without creating copies to pass to the drivers. However some drivers store the pointer to returned edid block somewhere in their own data structures, so if we do this it can easily get out of sync when we reread the edid and it has changed for whatever reason in which case the driver will have a stale and invalid pointer.
Agreed, that would be nice.
The whole thing is ugly, and needs to be revised. I've uploaded a patch to refactor this against the chromium tree, but haven't yet rebased against upstream. See https://gerrit.chromium.org/gerrit/#/c/38406/
For now, please drop this patch.
I've looked at the code in the exynos driver in the drm_next branch - there's nothing that destroys this edid data any more.
Yep, right you are. Your patch does the right thing, thanks for setting me straight :)
I hope we won't need to live with this double-allocation + copy for too long.
Sean
Cheers, Egbert.
dri-devel@lists.freedesktop.org
-
Egbert Eich -
Sean Paul