[v3,1/5] drm/dp: add connector backpointer to drm_dp_aux
diff mbox

Message ID 20170106143029.11553-2-tomeu.vizoso@collabora.com
State New
Headers show

Commit Message

Tomeu Vizoso Jan. 6, 2017, 2:30 p.m. UTC
This backpointer allows DP helpers to access the connector it's being
used for.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 include/drm/drm_dp_helper.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Sean Paul Jan. 6, 2017, 3:56 p.m. UTC | #1
On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> This backpointer allows DP helpers to access the connector it's being
> used for.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
>  include/drm/drm_dp_helper.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 55bbeb0ff594..4fa77b434594 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>   * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>   * @dev: pointer to struct device that is the parent for this AUX channel
> + * @connector: backpointer to connector that uses this AUX channel
>   * @hw_mutex: internal mutex used for locking transfers
>   * @transfer: transfers a message representing a single AUX transaction
>   *
> @@ -757,6 +758,7 @@ struct drm_dp_aux {
>         const char *name;
>         struct i2c_adapter ddc;
>         struct device *dev;
> +       struct drm_connector *connector;

Perhaps I'm misreading the series, but it seems like you could instead
add int crc_pipe along with the other crc fields. Then when you start
the crc, set the pipe number. If you have the pipe in the crc worker,
you can wait vblank on that pipe (no pointers needed).

Reasonable?

Sean

>         struct mutex hw_mutex;
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);
> --
> 2.9.3
>
Tomeu Vizoso Jan. 9, 2017, 9:03 a.m. UTC | #2
On 6 January 2017 at 16:56, Sean Paul <seanpaul@chromium.org> wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> This backpointer allows DP helpers to access the connector it's being
>> used for.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>>  include/drm/drm_dp_helper.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index 55bbeb0ff594..4fa77b434594 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
>>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>>   * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>>   * @dev: pointer to struct device that is the parent for this AUX channel
>> + * @connector: backpointer to connector that uses this AUX channel
>>   * @hw_mutex: internal mutex used for locking transfers
>>   * @transfer: transfers a message representing a single AUX transaction
>>   *
>> @@ -757,6 +758,7 @@ struct drm_dp_aux {
>>         const char *name;
>>         struct i2c_adapter ddc;
>>         struct device *dev;
>> +       struct drm_connector *connector;
>
> Perhaps I'm misreading the series, but it seems like you could instead
> add int crc_pipe along with the other crc fields. Then when you start
> the crc, set the pipe number. If you have the pipe in the crc worker,
> you can wait vblank on that pipe (no pointers needed).
>
> Reasonable?

I think that would work fine, but is it any better? I like that the
connector isn't going to change during the lifetime of the drm_dp_aux,
but crc_pipe could be changing between sampling sessions and any bugs
in keeping that field updated would be quite disconcerting and
cumbersome to debug.

crc_pipe's advantage is that we wouldn't need to update all callers of
drm_dp_aux_register, but I think it's better to have a connector field
that is mistakenly NULL, than a crc_pipe field with the wrong value.

Regards,

Tomeu

> Sean
>
>>         struct mutex hw_mutex;
>>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>>                             struct drm_dp_aux_msg *msg);
>> --
>> 2.9.3
>>
>
>
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
Sean Paul Jan. 9, 2017, 4:43 p.m. UTC | #3
On Mon, Jan 9, 2017 at 4:03 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> On 6 January 2017 at 16:56, Sean Paul <seanpaul@chromium.org> wrote:
>> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> This backpointer allows DP helpers to access the connector it's being
>>> used for.
>>>
>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> ---
>>>
>>>  include/drm/drm_dp_helper.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>>> index 55bbeb0ff594..4fa77b434594 100644
>>> --- a/include/drm/drm_dp_helper.h
>>> +++ b/include/drm/drm_dp_helper.h
>>> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
>>>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>>>   * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>>>   * @dev: pointer to struct device that is the parent for this AUX channel
>>> + * @connector: backpointer to connector that uses this AUX channel
>>>   * @hw_mutex: internal mutex used for locking transfers
>>>   * @transfer: transfers a message representing a single AUX transaction
>>>   *
>>> @@ -757,6 +758,7 @@ struct drm_dp_aux {
>>>         const char *name;
>>>         struct i2c_adapter ddc;
>>>         struct device *dev;
>>> +       struct drm_connector *connector;
>>
>> Perhaps I'm misreading the series, but it seems like you could instead
>> add int crc_pipe along with the other crc fields. Then when you start
>> the crc, set the pipe number. If you have the pipe in the crc worker,
>> you can wait vblank on that pipe (no pointers needed).
>>
>> Reasonable?
>
> I think that would work fine, but is it any better? I like that the
> connector isn't going to change during the lifetime of the drm_dp_aux,

Is this actually guaranteed, though? I am having a hard time thinking
about a practical example where it's not implemented this way, but I
don't see anything actually enforcing the lifetime relationship.

> but crc_pipe could be changing between sampling sessions and any bugs
> in keeping that field updated would be quite disconcerting and
> cumbersome to debug.

Couldn't the same could be said for connector->state->crtc?

Sean

>
> crc_pipe's advantage is that we wouldn't need to update all callers of
> drm_dp_aux_register, but I think it's better to have a connector field
> that is mistakenly NULL, than a crc_pipe field with the wrong value.
>
> Regards,
>
> Tomeu
>
>> Sean
>>
>>>         struct mutex hw_mutex;
>>>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>>>                             struct drm_dp_aux_msg *msg);
>>> --
>>> 2.9.3
>>>
>>
>>
>>
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS

Patch
diff mbox

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 55bbeb0ff594..4fa77b434594 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -721,6 +721,7 @@  struct drm_dp_aux_msg {
  * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
  * @ddc: I2C adapter that can be used for I2C-over-AUX communication
  * @dev: pointer to struct device that is the parent for this AUX channel
+ * @connector: backpointer to connector that uses this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
  * @transfer: transfers a message representing a single AUX transaction
  *
@@ -757,6 +758,7 @@  struct drm_dp_aux {
 	const char *name;
 	struct i2c_adapter ddc;
 	struct device *dev;
+	struct drm_connector *connector;
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);