All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.
Date: Mon, 13 Mar 2017 12:55:31 +0200	[thread overview]
Message-ID: <20170313105531.GH31595@intel.com> (raw)
In-Reply-To: <a6b149cd-56ad-dc4a-acb6-18d64bf21c17@linux.intel.com>

On Mon, Mar 13, 2017 at 11:38:33AM +0100, Maarten Lankhorst wrote:
> 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@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?

Does it matter? Somewhere around the swap_state I suppose?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2017-03-13 10:55 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-09 13:06 [PATCH 0/3] drm/i915: Convert tv/dp_mst and crt " Maarten Lankhorst
2017-03-09 13:06 ` [PATCH 1/3] drm/i915: Convert intel_tv " Maarten Lankhorst
2017-03-09 17:36   ` Ville Syrjälä
2017-03-13  9:22     ` Maarten Lankhorst
2017-03-13  9:29       ` Ville Syrjälä
2017-03-13 10:38         ` Maarten Lankhorst
2017-03-13 10:55           ` Ville Syrjälä [this message]
2017-03-13 12:19             ` Maarten Lankhorst
2017-03-13 16:10             ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v2 Maarten Lankhorst
2017-03-13 16:22               ` Ville Syrjälä
2017-03-13 16:35                 ` Maarten Lankhorst
2017-03-13 16:44                   ` Ville Syrjälä
2017-03-13 17:14                     ` Maarten Lankhorst
2017-03-13 16:44                 ` Maarten Lankhorst
2017-03-14 18:36               ` Ville Syrjälä
2017-03-15  8:32                 ` Maarten Lankhorst
2017-03-15 10:08                 ` [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic, v3 Maarten Lankhorst
2017-03-16 10:48                   ` Daniel Vetter
2017-03-16 12:05                     ` Maarten Lankhorst
2017-03-16 14:56                       ` Daniel Vetter
2017-03-16 15:27                         ` Maarten Lankhorst
2017-03-16 15:43                           ` Daniel Vetter
2017-03-09 13:06 ` [PATCH 2/3] drm/i915: Convert intel_dp_mst connector properties to atomic Maarten Lankhorst
2017-03-09 13:06 ` [PATCH 3/3] drm/i915: Convert intel_crt " Maarten Lankhorst
2017-03-09 16:23 ` ✓ Fi.CI.BAT: success for drm/i915: Convert tv/dp_mst and crt " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170313105531.GH31595@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --subject='Re: [PATCH 1/3] drm/i915: Convert intel_tv connector properties to atomic.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.