| Message ID | 1464964636-3877-7-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 03, 2016 at 03:36:49PM +0100, Chris Wilson wrote: > When trying to split up the initialisation phase and the registration > phase, one immediate problem encountered is trying to use our own i2c > devices before registration with userspace (to read EDID during device > discovery). drm_dp_aux in particular only offers an interface for setting > up the device *after* we have exposed the connector via sysfs. In order > to break the chicken-and-egg problem, export drm_dp_aux_init() to > minimally prepare the i2c device for internal use before > drm_connector_register(). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Rafael Antognolli <rafael.antognolli@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++----- > include/drm/drm_dp_helper.h | 1 + > 2 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 4b088afa21b2..9b4ec65e1de6 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags) > } > > /** > - * drm_dp_aux_register() - initialise and register aux channel > + * drm_dp_aux_init() - minimally initialise an aux channel > * @aux: DisplayPort AUX channel > * > - * Returns 0 on success or a negative error code on failure. > + * If you need to use the drm_dp_aux's i2c adapter prior to registering it > + * with the outside world, call drm_dp_aux_init() first. You must still > + * call drm_dp_aux_register() once the connector has been registered to > + * allow userspace access to the auxiliary DP channel. > */ > -int drm_dp_aux_register(struct drm_dp_aux *aux) > +void drm_dp_aux_init(struct drm_dp_aux *aux) > { > - int ret; > - > mutex_init(&aux->hw_mutex); > > aux->ddc.algo = &drm_dp_i2c_algo; > @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux) > aux->ddc.lock_bus = lock_bus; > aux->ddc.trylock_bus = trylock_bus; > aux->ddc.unlock_bus = unlock_bus; > +} > +EXPORT_SYMBOL(drm_dp_aux_init); This doesn't feel very safe to me. To me it looks like the i2c core wasn't designed to have the adapter be used before i2c_add_adapter() is called. I guess it might work in this case since you provide your own lock vfuncs. I think someone should fix the i2c core to split i2c_add_adapter() & co. into init and register phases. Cc:ing i2c folks... > + > +/** > + * drm_dp_aux_register() - initialise and register aux channel > + * @aux: DisplayPort AUX channel > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int drm_dp_aux_register(struct drm_dp_aux *aux) > +{ > + int ret; > + > + if (!aux->ddc.algo) > + drm_dp_aux_init(aux); > > aux->ddc.class = I2C_CLASS_DDC; > aux->ddc.owner = THIS_MODULE; > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 5a848e734422..4d85cf2874af 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -805,6 +805,7 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link); > int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link); > int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link); > > +void drm_dp_aux_init(struct drm_dp_aux *aux); > int drm_dp_aux_register(struct drm_dp_aux *aux); > void drm_dp_aux_unregister(struct drm_dp_aux *aux); > > -- > 2.8.1
On Fri, Jun 03, 2016 at 05:59:11PM +0300, Ville Syrjälä wrote: > On Fri, Jun 03, 2016 at 03:36:49PM +0100, Chris Wilson wrote: > > When trying to split up the initialisation phase and the registration > > phase, one immediate problem encountered is trying to use our own i2c > > devices before registration with userspace (to read EDID during device > > discovery). drm_dp_aux in particular only offers an interface for setting > > up the device *after* we have exposed the connector via sysfs. In order > > to break the chicken-and-egg problem, export drm_dp_aux_init() to > > minimally prepare the i2c device for internal use before > > drm_connector_register(). > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Dave Airlie <airlied@redhat.com> > > Cc: Rafael Antognolli <rafael.antognolli@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: dri-devel@lists.freedesktop.org > > --- > > drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++----- > > include/drm/drm_dp_helper.h | 1 + > > 2 files changed, 22 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > index 4b088afa21b2..9b4ec65e1de6 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags) > > } > > > > /** > > - * drm_dp_aux_register() - initialise and register aux channel > > + * drm_dp_aux_init() - minimally initialise an aux channel > > * @aux: DisplayPort AUX channel > > * > > - * Returns 0 on success or a negative error code on failure. > > + * If you need to use the drm_dp_aux's i2c adapter prior to registering it > > + * with the outside world, call drm_dp_aux_init() first. You must still > > + * call drm_dp_aux_register() once the connector has been registered to > > + * allow userspace access to the auxiliary DP channel. > > */ > > -int drm_dp_aux_register(struct drm_dp_aux *aux) > > +void drm_dp_aux_init(struct drm_dp_aux *aux) > > { > > - int ret; > > - > > mutex_init(&aux->hw_mutex); > > > > aux->ddc.algo = &drm_dp_i2c_algo; > > @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux) > > aux->ddc.lock_bus = lock_bus; > > aux->ddc.trylock_bus = trylock_bus; > > aux->ddc.unlock_bus = unlock_bus; > > +} > > +EXPORT_SYMBOL(drm_dp_aux_init); > > This doesn't feel very safe to me. To me it looks like the i2c core > wasn't designed to have the adapter be used before i2c_add_adapter() > is called. I guess it might work in this case since you provide your > own lock vfuncs. > > I think someone should fix the i2c core to split i2c_add_adapter() > & co. into init and register phases. Cc:ing i2c folks... As you've seen, I sent the patches to split i2c_add_adapter() to allow for us to call i2c_init_adapter() here instead. It still requires the same basic review that this init (the same as above) is sufficient for using i2c_transfer(). I would like to get the regression fix completed without much futher ado - in particular, not depending upon landing an external patch. -Chris
On Thu, Jun 09, 2016 at 09:57:24PM +0100, Chris Wilson wrote: > On Fri, Jun 03, 2016 at 05:59:11PM +0300, Ville Syrjälä wrote: > > On Fri, Jun 03, 2016 at 03:36:49PM +0100, Chris Wilson wrote: > > > When trying to split up the initialisation phase and the registration > > > phase, one immediate problem encountered is trying to use our own i2c > > > devices before registration with userspace (to read EDID during device > > > discovery). drm_dp_aux in particular only offers an interface for setting > > > up the device *after* we have exposed the connector via sysfs. In order > > > to break the chicken-and-egg problem, export drm_dp_aux_init() to > > > minimally prepare the i2c device for internal use before > > > drm_connector_register(). > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Dave Airlie <airlied@redhat.com> > > > Cc: Rafael Antognolli <rafael.antognolli@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: dri-devel@lists.freedesktop.org > > > --- > > > drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++----- > > > include/drm/drm_dp_helper.h | 1 + > > > 2 files changed, 22 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > index 4b088afa21b2..9b4ec65e1de6 100644 > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags) > > > } > > > > > > /** > > > - * drm_dp_aux_register() - initialise and register aux channel > > > + * drm_dp_aux_init() - minimally initialise an aux channel > > > * @aux: DisplayPort AUX channel > > > * > > > - * Returns 0 on success or a negative error code on failure. > > > + * If you need to use the drm_dp_aux's i2c adapter prior to registering it > > > + * with the outside world, call drm_dp_aux_init() first. You must still > > > + * call drm_dp_aux_register() once the connector has been registered to > > > + * allow userspace access to the auxiliary DP channel. > > > */ > > > -int drm_dp_aux_register(struct drm_dp_aux *aux) > > > +void drm_dp_aux_init(struct drm_dp_aux *aux) > > > { > > > - int ret; > > > - > > > mutex_init(&aux->hw_mutex); > > > > > > aux->ddc.algo = &drm_dp_i2c_algo; > > > @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux) > > > aux->ddc.lock_bus = lock_bus; > > > aux->ddc.trylock_bus = trylock_bus; > > > aux->ddc.unlock_bus = unlock_bus; > > > +} > > > +EXPORT_SYMBOL(drm_dp_aux_init); > > > > This doesn't feel very safe to me. To me it looks like the i2c core > > wasn't designed to have the adapter be used before i2c_add_adapter() > > is called. I guess it might work in this case since you provide your > > own lock vfuncs. > > > > I think someone should fix the i2c core to split i2c_add_adapter() > > & co. into init and register phases. Cc:ing i2c folks... > > As you've seen, I sent the patches to split i2c_add_adapter() to allow for > us to call i2c_init_adapter() here instead. It still requires the same > basic review that this init (the same as above) is sufficient for using > i2c_transfer(). > > I would like to get the regression fix completed without much futher > ado - in particular, not depending upon landing an external patch. What regression is this?
On Fri, Jun 10, 2016 at 01:26:24PM +0300, Ville Syrjälä wrote: > On Thu, Jun 09, 2016 at 09:57:24PM +0100, Chris Wilson wrote: > > On Fri, Jun 03, 2016 at 05:59:11PM +0300, Ville Syrjälä wrote: > > > On Fri, Jun 03, 2016 at 03:36:49PM +0100, Chris Wilson wrote: > > > > When trying to split up the initialisation phase and the registration > > > > phase, one immediate problem encountered is trying to use our own i2c > > > > devices before registration with userspace (to read EDID during device > > > > discovery). drm_dp_aux in particular only offers an interface for setting > > > > up the device *after* we have exposed the connector via sysfs. In order > > > > to break the chicken-and-egg problem, export drm_dp_aux_init() to > > > > minimally prepare the i2c device for internal use before > > > > drm_connector_register(). > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Dave Airlie <airlied@redhat.com> > > > > Cc: Rafael Antognolli <rafael.antognolli@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: dri-devel@lists.freedesktop.org > > > > --- > > > > drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++----- > > > > include/drm/drm_dp_helper.h | 1 + > > > > 2 files changed, 22 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > > index 4b088afa21b2..9b4ec65e1de6 100644 > > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > > @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags) > > > > } > > > > > > > > /** > > > > - * drm_dp_aux_register() - initialise and register aux channel > > > > + * drm_dp_aux_init() - minimally initialise an aux channel > > > > * @aux: DisplayPort AUX channel > > > > * > > > > - * Returns 0 on success or a negative error code on failure. > > > > + * If you need to use the drm_dp_aux's i2c adapter prior to registering it > > > > + * with the outside world, call drm_dp_aux_init() first. You must still > > > > + * call drm_dp_aux_register() once the connector has been registered to > > > > + * allow userspace access to the auxiliary DP channel. > > > > */ > > > > -int drm_dp_aux_register(struct drm_dp_aux *aux) > > > > +void drm_dp_aux_init(struct drm_dp_aux *aux) > > > > { > > > > - int ret; > > > > - > > > > mutex_init(&aux->hw_mutex); > > > > > > > > aux->ddc.algo = &drm_dp_i2c_algo; > > > > @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux) > > > > aux->ddc.lock_bus = lock_bus; > > > > aux->ddc.trylock_bus = trylock_bus; > > > > aux->ddc.unlock_bus = unlock_bus; > > > > +} > > > > +EXPORT_SYMBOL(drm_dp_aux_init); > > > > > > This doesn't feel very safe to me. To me it looks like the i2c core > > > wasn't designed to have the adapter be used before i2c_add_adapter() > > > is called. I guess it might work in this case since you provide your > > > own lock vfuncs. > > > > > > I think someone should fix the i2c core to split i2c_add_adapter() > > > & co. into init and register phases. Cc:ing i2c folks... > > > > As you've seen, I sent the patches to split i2c_add_adapter() to allow for > > us to call i2c_init_adapter() here instead. It still requires the same > > basic review that this init (the same as above) is sufficient for using > > i2c_transfer(). > > > > I would like to get the regression fix completed without much futher > > ado - in particular, not depending upon landing an external patch. > > What regression is this? It all started from the unclaimed mmio access across suspend, which stems from never having touched a context before suspend. That in turn meant touching the delayed rps initialisation which ended up with Daniel saying that the entire init chain needs to be unravelled. Snowball. -Chris
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 4b088afa21b2..9b4ec65e1de6 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -791,15 +791,16 @@ static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags) } /** - * drm_dp_aux_register() - initialise and register aux channel + * drm_dp_aux_init() - minimally initialise an aux channel * @aux: DisplayPort AUX channel * - * Returns 0 on success or a negative error code on failure. + * If you need to use the drm_dp_aux's i2c adapter prior to registering it + * with the outside world, call drm_dp_aux_init() first. You must still + * call drm_dp_aux_register() once the connector has been registered to + * allow userspace access to the auxiliary DP channel. */ -int drm_dp_aux_register(struct drm_dp_aux *aux) +void drm_dp_aux_init(struct drm_dp_aux *aux) { - int ret; - mutex_init(&aux->hw_mutex); aux->ddc.algo = &drm_dp_i2c_algo; @@ -809,6 +810,21 @@ int drm_dp_aux_register(struct drm_dp_aux *aux) aux->ddc.lock_bus = lock_bus; aux->ddc.trylock_bus = trylock_bus; aux->ddc.unlock_bus = unlock_bus; +} +EXPORT_SYMBOL(drm_dp_aux_init); + +/** + * drm_dp_aux_register() - initialise and register aux channel + * @aux: DisplayPort AUX channel + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_dp_aux_register(struct drm_dp_aux *aux) +{ + int ret; + + if (!aux->ddc.algo) + drm_dp_aux_init(aux); aux->ddc.class = I2C_CLASS_DDC; aux->ddc.owner = THIS_MODULE; diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 5a848e734422..4d85cf2874af 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -805,6 +805,7 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link); int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link); int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link); +void drm_dp_aux_init(struct drm_dp_aux *aux); int drm_dp_aux_register(struct drm_dp_aux *aux); void drm_dp_aux_unregister(struct drm_dp_aux *aux);
When trying to split up the initialisation phase and the registration phase, one immediate problem encountered is trying to use our own i2c devices before registration with userspace (to read EDID during device discovery). drm_dp_aux in particular only offers an interface for setting up the device *after* we have exposed the connector via sysfs. In order to break the chicken-and-egg problem, export drm_dp_aux_init() to minimally prepare the i2c device for internal use before drm_connector_register(). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Dave Airlie <airlied@redhat.com> Cc: Rafael Antognolli <rafael.antognolli@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/drm_dp_helper.c | 26 +++++++++++++++++++++----- include/drm/drm_dp_helper.h | 1 + 2 files changed, 22 insertions(+), 5 deletions(-)