Re: [Intel-gfx] [PATCH v6 06/12] drm/edid: refactor _drm_edid_connector_update() and rename

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 20 Dec 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Dec 20, 2022 at 02:52:01PM +0200, Jani Nikula wrote:
>> On Tue, 20 Dec 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Fri, Dec 16, 2022 at 06:00:20PM +0200, Jani Nikula wrote:
>> >> By moving update_display_info() out of _drm_edid_connector_update() we
>> >> make the function purely about adding modes.
>> >
>> > I don't think that's quite true. The 4:2:0 stuff still updates
>> > various display_info things from the mode parsing functions.
>> 
>> Right. I meant the top level. Will amend the commit message.
>
> So what's going to happen with the 4:2:0 stuff? Are we just clobbering
> it if/when someone calls the update_display_info() stuff w/o calling 
> add_modes()?

Don't do that then?

*sigh*

I don't know.

It's pretty much the same thing as only calling update_display_info(),
before adding modes, and looking at the relevant info fields *before*
the add modes call. That's probably already happening.

I really wanted to put this all together into one call so nobody could
do that silly stuff. But then for various reasons drivers want to not
only read the EDID but also parse it in ->detect, and ->detect gets
called a lot.

We can't keep adding modes to probed modes in ->detect all the time
without pruning them. I thought about trying to avoid adding duplicated
modes in probed modes list, but it looks like another rabbit hole with
no end in sight. Don't really want to go that route.

If we want to make this fool proof, one way to fix this would be to do
all the info changes in step 1, even the ones that are currently added
via add modes. Iterate through everything, but only do the info
changes. And in step 2 only add the modes to probed modes list.

In any case, the current state of affairs is bonkers already. There's
supposed to be 1) reading the EDID, 2) updating the info, and 3) adding
the modes. But both drm_get_edid() *and* drm_add_edid_modes() do the
display info and property update part too. We've just been adding calls
here and there to patch things up. Nobody cares. Just add more calls
updating stuff, and hope it'll be fine. That's gotta stop.


BR,
Jani.



>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> >> Rename accordingly.
>> >> 
>> >> Cc: Imre Deak <imre.deak@intel.com>
>> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_edid.c | 25 ++++++++++++-------------
>> >>  1 file changed, 12 insertions(+), 13 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> >> index 15f69c362fc3..4ebfd7212bce 100644
>> >> --- a/drivers/gpu/drm/drm_edid.c
>> >> +++ b/drivers/gpu/drm/drm_edid.c
>> >> @@ -6575,19 +6575,12 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>> >>  	return num_modes;
>> >>  }
>> >>  
>> >> -static int _drm_edid_connector_update(struct drm_connector *connector,
>> >> -				      const struct drm_edid *drm_edid)
>> >> +static int _drm_edid_connector_add_modes(struct drm_connector *connector,
>> >> +					 const struct drm_edid *drm_edid)
>> >>  {
>> >>  	const struct drm_display_info *info = &connector->display_info;
>> >>  	int num_modes = 0;
>> >>  
>> >> -	/*
>> >> -	 * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks.
>> >> -	 * To avoid multiple parsing of same block, lets parse that map
>> >> -	 * from sink info, before parsing CEA modes.
>> >> -	 */
>> >> -	update_display_info(connector, drm_edid);
>> >> -
>> >>  	if (!drm_edid)
>> >>  		return 0;
>> >>  
>> >> @@ -6692,7 +6685,9 @@ int drm_edid_connector_update(struct drm_connector *connector,
>> >>  {
>> >>  	int count;
>> >>  
>> >> -	count = _drm_edid_connector_update(connector, drm_edid);
>> >> +	update_display_info(connector, drm_edid);
>> >> +
>> >> +	count = _drm_edid_connector_add_modes(connector, drm_edid);
>> >>  
>> >>  	_drm_update_tile_info(connector, drm_edid);
>> >>  
>> >> @@ -6762,7 +6757,8 @@ EXPORT_SYMBOL(drm_connector_update_edid_property);
>> >>   */
>> >>  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>> >>  {
>> >> -	struct drm_edid drm_edid;
>> >> +	struct drm_edid _drm_edid;
>> >> +	const struct drm_edid *drm_edid;
>> >>  
>> >>  	if (edid && !drm_edid_is_valid(edid)) {
>> >>  		drm_warn(connector->dev, "[CONNECTOR:%d:%s] EDID invalid.\n",
>> >> @@ -6770,8 +6766,11 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>> >>  		edid = NULL;
>> >>  	}
>> >>  
>> >> -	return _drm_edid_connector_update(connector,
>> >> -					  drm_edid_legacy_init(&drm_edid, edid));
>> >> +	drm_edid = drm_edid_legacy_init(&_drm_edid, edid);
>> >> +
>> >> +	update_display_info(connector, drm_edid);
>> >> +
>> >> +	return _drm_edid_connector_add_modes(connector, drm_edid);
>> >>  }
>> >>  EXPORT_SYMBOL(drm_add_edid_modes);
>> >>  
>> >> -- 
>> >> 2.34.1
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center