On Fri, Oct 15, 2021 at 06:18:34PM +0300, Jani Nikula wrote:
> On Fri, 15 Oct 2021, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula wrote:
> >> On Fri, 15 Oct 2021, Claudio Suarez <cssk@xxxxxxxx> wrote:
> >> > Once EDID is parsed, the monitor HDMI support information is available
> >> > through drm_display_info.is_hdmi. Retriving the same information with
> >> > drm_detect_hdmi_monitor() is less efficient. Change to
> >> > drm_display_info.is_hdmi where possible.
> >> >
> >> > This is a TODO task in Documentation/gpu/todo.rst
> >> >
> >> > Signed-off-by: Claudio Suarez <cssk@xxxxxxxx>
> >> > ---
> >> > drivers/gpu/drm/i915/display/intel_connector.c | 5 +++++
> >> > drivers/gpu/drm/i915/display/intel_connector.h | 1 +
> >> > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> >> > drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> >> > 4 files changed, 9 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
> >> > index 9bed1ccecea0..3346b55df6e1 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_connector.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> >> > @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector,
> >> > return ret;
> >> > }
> >> >
> >> > +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector)
> >> > +{
> >> > + return connector->display_info.is_hdmi;
> >> > +}
> >> > +
> >>
> >> A helper like this belongs in drm, not i915. Seems useful in other
> >> drivers too.
> >
> > Not sure it's actually helpful for i915. We end up having to root around
> > in the display_info in a lot of places anyway. So a helper for single
> > boolean seems a bit out of place perhaps.
>
> *shrug*
>
> Maybe it's just my frustration at the lack of interfaces and poking
> around in the depths of nested structs and pointer chasing that's coming
> through. You just need to change so many things if you want to later
> refactor where "is hdmi" comes from and is stored.
>
> Anyway, if a helper is being added like in this series, I think it
> should be one helper in drm, not redundant copies in multiple
> drivers. Or we should not have the helper(s) at all. One or the other,
> not the worst of both worlds.
Thank you all for your comments :)
The big work here was to figure out which drm_detect_hdmi_monitor() can be
replaced. Changing a helper isn't a problem.
I'll send a new patch in a few hours.
BR
Claudio Suarez.
