[Intel-gfx] [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.

[Maarten Lankhorst]

Op 13-03-17 om 10:29 schreef Ville Syrjälä:
> On Mon, Mar 13, 2017 at 10:22:51AM +0100, Maarten Lankhorst wrote:
>> Op 09-03-17 om 18:36 schreef Ville Syrjälä:
>>> On Thu, Mar 09, 2017 at 02:06:15PM +0100, Maarten Lankhorst wrote:
>>>> As a proof of concept, first try to convert intel_tv, which is a rarely
>>>> used connector. It has 5 properties, tv format and 4 margins.
>>> Since it's so rare, if you want someone to actually test the code
>>> it'll probably make sense to pick another connector ;)
>> Yeah but the properties are among the most annoying, with the self modifying code and using state in mode_detect().
>>>> I'm less certain about the state behavior itself, should we pass a size
>>>> parameter to intel_connector_alloc instead, so duplicate_state
>>>> can be done globally if it can be blindly copied?
>>>>
>>>> Can we also have a atomic_check function for connectors, so the
>>>> crtc_state->connectors_changed can be set there? It would be cleaner
>>>> and more atomic-like.
>>> Hmm. I think it migth be really useful only if we have some
>>> interactions between multiple properties that really need to be
>>> checked. We might have those already I suppose but we don't seem
>>> to check any of it currently. So as a first step I guess we can
>>> just keep ignoring any such issues.
>> Well it might be, for example not all properties may be set yet so you can only do a sane check in a separate step.
>>>> To match the legacy behavior, format can be changed by probing just like
>>>> in legacy mode.
>>> Self modifying state irks me, but it's what we've been doing so I guess
>>> we should keep it.
>> Yeah, I hate it too.
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_tv.c | 238 +++++++++++++++++++++++-----------------
>>>>  1 file changed, 136 insertions(+), 102 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>>>> index 6ed1a3ce47b7..0fb1d8621fe8 100644
>>>> --- a/drivers/gpu/drm/i915/intel_tv.c
>>>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>>>> @@ -48,8 +48,6 @@ struct intel_tv {
>>>>  	struct intel_encoder base;
>>>>  
>>>>  	int type;
>>>> -	const char *tv_format;
>>>> -	int margin[4];
>>>>  	u32 save_TV_H_CTL_1;
>>>>  	u32 save_TV_H_CTL_2;
>>>>  	u32 save_TV_H_CTL_3;
>>>> @@ -85,6 +83,16 @@ struct intel_tv {
>>>>  	u32 save_TV_CTL;
>>>>  };
>>>>  
>>>> +struct intel_tv_connector_state {
>>>> +	struct drm_connector_state base;
>>>> +
>>>> +	int format;
>>>> +	int margin[4];
>>>> +};
>>>> +
>>>> +#define to_intel_tv_connector_state(state) \
>>>> +	container_of((state), struct intel_tv_connector_state, base)
>>>> +
>>>>  struct video_levels {
>>>>  	u16 blank, black;
>>>>  	u8 burst;
>>>> @@ -873,32 +881,18 @@ intel_disable_tv(struct intel_encoder *encoder,
>>>>  	I915_WRITE(TV_CTL, I915_READ(TV_CTL) & ~TV_ENC_ENABLE);
>>>>  }
>>>>  
>>>> -static const struct tv_mode *
>>>> -intel_tv_mode_lookup(const char *tv_format)
>>>> +static const struct tv_mode *intel_tv_mode_find(struct drm_connector_state *conn_state)
>>>>  {
>>>> -	int i;
>>>> +	int format = to_intel_tv_connector_state(conn_state)->format;
>>>>  
>>>> -	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
>>>> -		const struct tv_mode *tv_mode = &tv_modes[i];
>>>> -
>>>> -		if (!strcmp(tv_format, tv_mode->name))
>>>> -			return tv_mode;
>>>> -	}
>>>> -	return NULL;
>>>> -}
>>>> -
>>>> -static const struct tv_mode *
>>>> -intel_tv_mode_find(struct intel_tv *intel_tv)
>>>> -{
>>>> -	return intel_tv_mode_lookup(intel_tv->tv_format);
>>>> +	return &tv_modes[format];
>>>>  }
>>>>  
>>>>  static enum drm_mode_status
>>>>  intel_tv_mode_valid(struct drm_connector *connector,
>>>>  		    struct drm_display_mode *mode)
>>>>  {
>>>> -	struct intel_tv *intel_tv = intel_attached_tv(connector);
>>>> -	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>>>> +	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
>>> It feels a bit fishy to use the state here. Generally that's a no-no.
>>> But in this case I wonder if it's the right choice after all. 
>>>
>>> Not sure if some kind of "automatic" enum value might also work. It
>>> would at least avoid the self modifying property problem. Although I
>>> wonder if the user would still like to know what was actually used
>>> if they chose they automatic mode, so we might need a self modifying
>>> RO property for the current mode anyway.
>>>
>>> But that still leaves the problem of how the user would know which modes
>>> they should be able to use if .get_modes()/.mode_valid() doesn't respect
>>> the users choice of the tv format. Hmm, tricky. Might be the self
>>> modifying property is the only good choice.
>>>
>>> But if we would use the state here, what's the story with locking going
>>> to be? connection_mutex is what protects this stuff, but we're not
>>> holding that during mode enumeration.
>> Yeah locking is tricky, honestly I have no idea what would be the right thing to do here..
>>
>> I don't really see a good solution, or at least one that would work correctly with atomic properties.
> Maybe we need to keep the format information in both intel_tv and the
> state. We'd use the intel_tv->format during detect, and during
> duplicate_state we'd do 'state->format = intel_tv->format' and during
> commit we'd do 'intel_tv->format = state->format' ?
>
> Still self modifying, and somewhat racy still, but at least we
> shouldn't explode on account of the connector->state dereference.
>
I thought about that, but where do you want to update it in atomic commit?