On Tue, Nov 20, 2012 at 3:29 PM, Egbert Eich <eich@xxxxxxxx> wrote:
> Sean Paul writes:
> > On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich <eich@xxxxxxx> 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@xxxxxxx>
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > > ---
> > > 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 mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
