Re: [PATCH v3 1/9] drm/mediatek: dp: Cache EDID for eDP panel

[AngeloGioacchino Del Regno]

Il 12/04/23 09:08, Matthias Brugger ha scritto:
> On 04/04/2023 12:47, AngeloGioacchino Del Regno wrote:
> > Since eDP panels are not removable it is safe to cache the EDID:
> > this will avoid a relatively long read transaction at every PM
> > resume that is unnecessary only in the "special" case of eDP,
> > hence speeding it up a little, as from now on, as resume operation,
> > we will perform only link training.
> >
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/mediatek/mtk_dp.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> > index 1f94fcc144d3..84f82cc68672 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> > @@ -118,6 +118,7 @@ struct mtk_dp {
> >       const struct mtk_dp_data *data;
> >       struct mtk_dp_info info;
> >       struct mtk_dp_train_info train_info;
> > +    struct edid *edid;
> >       struct platform_device *phy_dev;
> >       struct phy *phy;
> > @@ -1993,7 +1994,11 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
> > *bridge,
> >           usleep_range(2000, 5000);
> >       }
> > -    new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> > +    /* eDP panels aren't removable, so we can return a cached EDID. */
> > +    if (mtk_dp->edid && mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP)
> > +        new_edid = drm_edid_duplicate(mtk_dp->edid);
> > +    else
> > +        new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
>
> Maybe it would make sense to add a macro for the check of mtk_dp->bridge.type == 
> DRM_MODE_CONNECTOR_eDP
> it would make the code more readable.

I had the same idea... but then avoided that because in most (if not all?) of the
DRM drivers (at least, the one I've read) this check is always open coded, so I
wrote it like that for consistency and nothing else.

I have no strong opinions on that though!

> >       /*
> >        * Parse capability here to let atomic_get_input_bus_fmts and
> > @@ -2022,6 +2027,10 @@ static struct edid *mtk_dp_get_edid(struct drm_bridge 
> > *bridge,
> >           drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
> >       }
> > +    /* If this is an eDP panel and the read EDID is good, cache it for later */
> > +    if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP && !mtk_dp->edid && new_edid)
> > +        mtk_dp->edid = drm_edid_duplicate(new_edid);
> > +
>
> How about putting this in an else if branch of mtk_dp_parse_capabilities. At least 
> we could get rid of the check regarding if new_edid != NULL.
>
> I was thinking on how to put both if statements in one block, but I think the 
> problem is, that we would leak memory if the capability parsing failes due to the 
> call to drm_edid_duplicate(). Correct?

Correct. The only other "good" place would be in the `if (new_edid)` conditional,
but that wouldn't be as readable as it is right now...

Cheers,
Angelo