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@xxxxxxxxxxxxxxx>
>> ---
>> 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
