Nouveau - Oct 2021 - [Intel-gfx] [PATCH 15/15] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi

On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula
wrote:
> On Fri, 15 Oct 2021, Claudio Suarez <cssk at net-c.es> 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 at net-c.es>
> > ---
> >  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.

-- 
Ville Syrj?l?
Intel

On Fri, 15 Oct 2021, Ville Syrj?l? <ville.syrjala at linux.intel.com>
wrote:
> On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula wrote:
>> On Fri, 15 Oct 2021, Claudio Suarez <cssk at net-c.es> 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 at net-c.es>
>> > ---
>> >  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.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center