Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210330025345.3980086-1-dianders@chromium.org>
 <20210329195255.v2.9.Ia7e9bb7cf6c51d960b9455fb0fa447cc68ece99d@changeid> <YGMvO3PNDCiBmqov@intel.com>
In-Reply-To: <YGMvO3PNDCiBmqov@intel.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 30 Mar 2021 14:31:41 -0700
Message-ID: <CAD=FV=UgnPMkJ6moLDvg8FPTKWUcTrp1mYED2i-K1vjJFu4cEA@mail.gmail.com>
Subject: Re: [PATCH v2 09/14] drm/edid: Use the cached EDID in drm_get_edid()
 if eDP
To:     =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>
Cc:     Andrzej Hajda <a.hajda@samsung.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@siol.net>,
        Sam Ravnborg <sam@ravnborg.org>,
        Rob Clark <robdclark@chromium.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        David Airlie <airlied@linux.ie>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Steev Klimaszewski <steev@kali.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Tue, Mar 30, 2021 at 7:01 AM Ville Syrj=C3=A4l=C3=A4
<ville.syrjala@linux.intel.com> wrote:
>
> > @@ -2049,15 +2049,39 @@ struct edid *drm_get_edid(struct drm_connector =
*connector,
> >                         struct i2c_adapter *adapter)
> >  {
> >       struct edid *edid;
> > +     size_t old_edid_size;
> > +     const struct edid *old_edid;
> >
> >       if (connector->force =3D=3D DRM_FORCE_OFF)
> >               return NULL;
> >
> > -     if (connector->force =3D=3D DRM_FORCE_UNSPECIFIED && !drm_probe_d=
dc(adapter))
> > -             return NULL;
> > +     if (connector->connector_type =3D=3D DRM_MODE_CONNECTOR_eDP &&
> > +         connector->edid_blob_ptr) {
> > +             /*
> > +              * eDP devices are non-removable, or at least not somethi=
ng
> > +              * that's expected to be hot-pluggable. We can freely use
> > +              * the cached EDID.
> > +              *
> > +              * NOTE: technically we could probably even use the cache=
d
> > +              * EDID even for non-eDP because the cached EDID should b=
e
> > +              * cleared if we ever notice a display is not connected, =
but
> > +              * we'll use an abundance of caution and only do it for e=
DP.
> > +              * It's more important for eDP anyway because the EDID ma=
y not
> > +              * always be readable, like when the panel is powered dow=
n.
> > +              */
> > +             old_edid =3D (const struct edid *)connector->edid_blob_pt=
r->data;
> > +             old_edid_size =3D ksize(old_edid);
> > +             edid =3D kmalloc(old_edid_size, GFP_KERNEL);
> > +             if (edid)
> > +                     memcpy(edid, old_edid, old_edid_size);
> > +     } else {
> > +             if (connector->force =3D=3D DRM_FORCE_UNSPECIFIED && !drm=
_probe_ddc(adapter))
> > +                     return NULL;
> > +
> > +             edid =3D drm_do_get_edid(connector, drm_do_probe_ddc_edid=
, adapter);
> > +             drm_connector_update_edid_property(connector, edid);
> > +     }
>
> This is a pretty low level function. Too low level for this caching
> IMO. So I think better just do it a bit higher up like other drivers.

Fair enough. In the past I'd gotten feedback that I'd been jamming too
much stuff in my own driver instead of putting it in the core, but I'm
happy to leave the EDID caching in the driver if that's what people
prefer. It actually makes a bit of the code in the driver a bit less
awkward...

-Doug