diff mbox

[3/5] drm/i2c: adv7511: Refactor encoder slave functions

Message ID 1437977819-24199-4-git-send-email-architt@codeaurora.org
State New, archived
Headers show

Commit Message

Archit Taneja July 27, 2015, 6:16 a.m. UTC
ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
the other hand, is going be a normal i2c client device creating bridge
and connector entities.

Move the code in encoder slave functions to generate helpers that are
agnostic to the drm object type. These helpers will later also be used
by bridge and connecter helper functions.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/i2c/adv7511.c | 80 ++++++++++++++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 23 deletions(-)

Comments

Laurent Pinchart July 27, 2015, 8:59 a.m. UTC | #1
Hi Archit,

(CC'ing Boris Brezillon)

Thank you for the patch.

On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
> the other hand, is going be a normal i2c client device creating bridge
> and connector entities.

Please, no. It's really time to stop piling hacks and fix the problem 
properly. There's no reason to have separate APIs for I2C slave encoders and 
DRM bridges. Those two APIs need to be merged, and then you'll find it much 
easier to implement ADV7533 support.

Boris, I know you were experimenting with that, do you have anything to report 
?

> Move the code in encoder slave functions to generate helpers that are
> agnostic to the drm object type. These helpers will later also be used
> by bridge and connecter helper functions.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/i2c/adv7511.c | 80 +++++++++++++++++++++++++++------------
>  1 file changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
> index 63a3d20..46fb24d 100644
> --- a/drivers/gpu/drm/i2c/adv7511.c
> +++ b/drivers/gpu/drm/i2c/adv7511.c
> @@ -612,13 +612,11 @@ static int adv7511_get_edid_block(void *data, u8 *buf,
> unsigned int block, }
> 
>  /*
> ---------------------------------------------------------------------------
> -- - * Encoder operations
> + * ADV75xx helpers
>   */
> -
> -static int adv7511_get_modes(struct drm_encoder *encoder,
> -			     struct drm_connector *connector)
> +static int adv7511_get_modes(struct adv7511 *adv7511,
> +		struct drm_connector *connector)
>  {
> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>  	struct edid *edid;
>  	unsigned int count;
> 
> @@ -656,21 +654,10 @@ static int adv7511_get_modes(struct drm_encoder
> *encoder, return count;
>  }
> 
> -static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
> -{
> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> -
> -	if (mode == DRM_MODE_DPMS_ON)
> -		adv7511_power_on(adv7511);
> -	else
> -		adv7511_power_off(adv7511);
> -}
> -
>  static enum drm_connector_status
> -adv7511_encoder_detect(struct drm_encoder *encoder,
> +adv7511_detect(struct adv7511 *adv7511,
>  		       struct drm_connector *connector)
>  {
> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>  	enum drm_connector_status status;
>  	unsigned int val;
>  	bool hpd;
> @@ -694,7 +681,7 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
>  	if (status == connector_status_connected && hpd && adv7511->powered) {
>  		regcache_mark_dirty(adv7511->regmap);
>  		adv7511_power_on(adv7511);
> -		adv7511_get_modes(encoder, connector);
> +		adv7511_get_modes(adv7511, connector);
>  		if (adv7511->status == connector_status_connected)
>  			status = connector_status_disconnected;
>  	} else {
> @@ -708,8 +695,8 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
>  	return status;
>  }
> 
> -static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
> -				      struct drm_display_mode *mode)
> +static int adv7511_mode_valid(struct adv7511 *adv7511,
> +				     const struct drm_display_mode *mode)
>  {
>  	if (mode->clock > 165000)
>  		return MODE_CLOCK_HIGH;
> @@ -717,11 +704,10 @@ static int adv7511_encoder_mode_valid(struct
> drm_encoder *encoder, return MODE_OK;
>  }
> 
> -static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
> +static void adv7511_mode_set(struct adv7511 *adv7511,
>  				     struct drm_display_mode *mode,
>  				     struct drm_display_mode *adj_mode)
>  {
> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>  	unsigned int low_refresh_rate;
>  	unsigned int hsync_polarity = 0;
>  	unsigned int vsync_polarity = 0;
> @@ -812,12 +798,60 @@ static void adv7511_encoder_mode_set(struct
> drm_encoder *encoder, adv7511->f_tmds = mode->clock;
>  }
> 
> +/*
> ---------------------------------------------------------------------------
> -- + * Encoder operations
> + */
> +
> +static int adv7511_encoder_get_modes(struct drm_encoder *encoder,
> +			     struct drm_connector *connector)
> +{
> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> +
> +	return adv7511_get_modes(adv7511, connector);
> +}
> +
> +static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
> +{
> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> +
> +	if (mode == DRM_MODE_DPMS_ON)
> +		adv7511_power_on(adv7511);
> +	else
> +		adv7511_power_off(adv7511);
> +}
> +
> +static enum drm_connector_status
> +adv7511_encoder_detect(struct drm_encoder *encoder,
> +		       struct drm_connector *connector)
> +{
> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> +
> +	return adv7511_detect(adv7511, connector);
> +}
> +
> +static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
> +				      struct drm_display_mode *mode)
> +{
> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> +
> +	return adv7511_mode_valid(adv7511, mode);
> +}
> +
> +static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
> +				     struct drm_display_mode *mode,
> +				     struct drm_display_mode *adj_mode)
> +{
> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> +
> +	adv7511_mode_set(adv7511, mode, adj_mode);
> +}
> +
>  static struct drm_encoder_slave_funcs adv7511_encoder_funcs = {
>  	.dpms = adv7511_encoder_dpms,
>  	.mode_valid = adv7511_encoder_mode_valid,
>  	.mode_set = adv7511_encoder_mode_set,
>  	.detect = adv7511_encoder_detect,
> -	.get_modes = adv7511_get_modes,
> +	.get_modes = adv7511_encoder_get_modes,
>  };
> 
>  /*
> ---------------------------------------------------------------------------
> --
Archit Taneja July 28, 2015, 8:17 a.m. UTC | #2
Hi,

On 07/27/2015 02:29 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> (CC'ing Boris Brezillon)
>
> Thank you for the patch.
>
> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
>> the other hand, is going be a normal i2c client device creating bridge
>> and connector entities.
>
> Please, no. It's really time to stop piling hacks and fix the problem
> properly. There's no reason to have separate APIs for I2C slave encoders and
> DRM bridges. Those two APIs need to be merged, and then you'll find it much
> easier to implement ADV7533 support.

i2c slave encoders and bridges aren't exactly the same. slave encoders
are used when the there is no real 'encoder' in the display chain.
bridges are used when there is already an encoder available, and the
bridge entity represents another encoder in the chain.

ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a 
crtc for drm drivers.

ADV7533 takes in MIPI DSI data, which is generally the output of an
encoder for drm drivers.

Therefore, having i2c slave encoder for the former and bridge for the
latter made sense to me.

I do agree that it would be better if they were somehow merged. It
would prevent the fragmentation we currently have among encoder
chips.

One possible way would be to convert slave encoder to bridge. With
this, an i2c slave encoder would be a 'dummy encoder' and a bridge.
i2c slave encoders even now just tie the slave encoder helper funcs
to encoder helper funcs. So it's not really any different.

Merging these 2 entities would be nice, but we're still shying away
from the larger problem of creating generic encoder chains. The
ideal solution would be for bridges and slave encoders to just be
'encoders', and the facility to connect on encoder output to the
input of another. I don't know how easy it is to do this, and
whether it'll break userspace.

Archit

>
> Boris, I know you were experimenting with that, do you have anything to report
> ?
>
>> Move the code in encoder slave functions to generate helpers that are
>> agnostic to the drm object type. These helpers will later also be used
>> by bridge and connecter helper functions.
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>   drivers/gpu/drm/i2c/adv7511.c | 80 +++++++++++++++++++++++++++------------
>>   1 file changed, 57 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
>> index 63a3d20..46fb24d 100644
>> --- a/drivers/gpu/drm/i2c/adv7511.c
>> +++ b/drivers/gpu/drm/i2c/adv7511.c
>> @@ -612,13 +612,11 @@ static int adv7511_get_edid_block(void *data, u8 *buf,
>> unsigned int block, }
>>
>>   /*
>> ---------------------------------------------------------------------------
>> -- - * Encoder operations
>> + * ADV75xx helpers
>>    */
>> -
>> -static int adv7511_get_modes(struct drm_encoder *encoder,
>> -			     struct drm_connector *connector)
>> +static int adv7511_get_modes(struct adv7511 *adv7511,
>> +		struct drm_connector *connector)
>>   {
>> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>   	struct edid *edid;
>>   	unsigned int count;
>>
>> @@ -656,21 +654,10 @@ static int adv7511_get_modes(struct drm_encoder
>> *encoder, return count;
>>   }
>>
>> -static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
>> -{
>> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> -
>> -	if (mode == DRM_MODE_DPMS_ON)
>> -		adv7511_power_on(adv7511);
>> -	else
>> -		adv7511_power_off(adv7511);
>> -}
>> -
>>   static enum drm_connector_status
>> -adv7511_encoder_detect(struct drm_encoder *encoder,
>> +adv7511_detect(struct adv7511 *adv7511,
>>   		       struct drm_connector *connector)
>>   {
>> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>   	enum drm_connector_status status;
>>   	unsigned int val;
>>   	bool hpd;
>> @@ -694,7 +681,7 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
>>   	if (status == connector_status_connected && hpd && adv7511->powered) {
>>   		regcache_mark_dirty(adv7511->regmap);
>>   		adv7511_power_on(adv7511);
>> -		adv7511_get_modes(encoder, connector);
>> +		adv7511_get_modes(adv7511, connector);
>>   		if (adv7511->status == connector_status_connected)
>>   			status = connector_status_disconnected;
>>   	} else {
>> @@ -708,8 +695,8 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
>>   	return status;
>>   }
>>
>> -static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
>> -				      struct drm_display_mode *mode)
>> +static int adv7511_mode_valid(struct adv7511 *adv7511,
>> +				     const struct drm_display_mode *mode)
>>   {
>>   	if (mode->clock > 165000)
>>   		return MODE_CLOCK_HIGH;
>> @@ -717,11 +704,10 @@ static int adv7511_encoder_mode_valid(struct
>> drm_encoder *encoder, return MODE_OK;
>>   }
>>
>> -static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
>> +static void adv7511_mode_set(struct adv7511 *adv7511,
>>   				     struct drm_display_mode *mode,
>>   				     struct drm_display_mode *adj_mode)
>>   {
>> -	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>   	unsigned int low_refresh_rate;
>>   	unsigned int hsync_polarity = 0;
>>   	unsigned int vsync_polarity = 0;
>> @@ -812,12 +798,60 @@ static void adv7511_encoder_mode_set(struct
>> drm_encoder *encoder, adv7511->f_tmds = mode->clock;
>>   }
>>
>> +/*
>> ---------------------------------------------------------------------------
>> -- + * Encoder operations
>> + */
>> +
>> +static int adv7511_encoder_get_modes(struct drm_encoder *encoder,
>> +			     struct drm_connector *connector)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	return adv7511_get_modes(adv7511, connector);
>> +}
>> +
>> +static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	if (mode == DRM_MODE_DPMS_ON)
>> +		adv7511_power_on(adv7511);
>> +	else
>> +		adv7511_power_off(adv7511);
>> +}
>> +
>> +static enum drm_connector_status
>> +adv7511_encoder_detect(struct drm_encoder *encoder,
>> +		       struct drm_connector *connector)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	return adv7511_detect(adv7511, connector);
>> +}
>> +
>> +static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
>> +				      struct drm_display_mode *mode)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	return adv7511_mode_valid(adv7511, mode);
>> +}
>> +
>> +static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
>> +				     struct drm_display_mode *mode,
>> +				     struct drm_display_mode *adj_mode)
>> +{
>> +	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +	adv7511_mode_set(adv7511, mode, adj_mode);
>> +}
>> +
>>   static struct drm_encoder_slave_funcs adv7511_encoder_funcs = {
>>   	.dpms = adv7511_encoder_dpms,
>>   	.mode_valid = adv7511_encoder_mode_valid,
>>   	.mode_set = adv7511_encoder_mode_set,
>>   	.detect = adv7511_encoder_detect,
>> -	.get_modes = adv7511_get_modes,
>> +	.get_modes = adv7511_encoder_get_modes,
>>   };
>>
>>   /*
>> ---------------------------------------------------------------------------
>> --
>
Boris BREZILLON July 28, 2015, 2:38 p.m. UTC | #3
Archit, Laurent,

On Tue, 28 Jul 2015 13:47:37 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> Hi,
> 
> On 07/27/2015 02:29 PM, Laurent Pinchart wrote:
> > Hi Archit,
> >
> > (CC'ing Boris Brezillon)
> >
> > Thank you for the patch.
> >
> > On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
> >> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
> >> the other hand, is going be a normal i2c client device creating bridge
> >> and connector entities.
> >
> > Please, no. It's really time to stop piling hacks and fix the problem
> > properly. There's no reason to have separate APIs for I2C slave encoders and
> > DRM bridges. Those two APIs need to be merged, and then you'll find it much
> > easier to implement ADV7533 support.
> 
> i2c slave encoders and bridges aren't exactly the same. slave encoders
> are used when the there is no real 'encoder' in the display chain.
> bridges are used when there is already an encoder available, and the
> bridge entity represents another encoder in the chain.
> 
> ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a 
> crtc for drm drivers.
> 
> ADV7533 takes in MIPI DSI data, which is generally the output of an
> encoder for drm drivers.
> 
> Therefore, having i2c slave encoder for the former and bridge for the
> latter made sense to me.
> 
> I do agree that it would be better if they were somehow merged. It
> would prevent the fragmentation we currently have among encoder
> chips.
> 
> One possible way would be to convert slave encoder to bridge. With
> this, an i2c slave encoder would be a 'dummy encoder' and a bridge.
> i2c slave encoders even now just tie the slave encoder helper funcs
> to encoder helper funcs. So it's not really any different.
> 
> Merging these 2 entities would be nice, but we're still shying away
> from the larger problem of creating generic encoder chains. The
> ideal solution would be for bridges and slave encoders to just be
> 'encoders', and the facility to connect on encoder output to the
> input of another. I don't know how easy it is to do this, and
> whether it'll break userspace.

Yes, that's pretty much what I was trying to do.
I'd also like to ease display pipelines creation by providing helper
functions, so that display controller don't have to worry about encoders
and connectors creation if these ones are attached to external encoders.

> 
> Archit
> 
> >
> > Boris, I know you were experimenting with that, do you have anything to report
> > ?

Nope, I didn't work on it since last time we talked about it. I pushed
my work here if you want to have a look [1].

Best Regards,

Boris

[1]https://github.com/bbrezillon/linux-at91/tree/drm-encoder-chain-WIP
Archit Taneja July 31, 2015, 5:26 a.m. UTC | #4
Hi Boris, Laurent,

On 07/28/2015 08:08 PM, Boris Brezillon wrote:
> Archit, Laurent,
>
> On Tue, 28 Jul 2015 13:47:37 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
>
>> Hi,
>>
>> On 07/27/2015 02:29 PM, Laurent Pinchart wrote:
>>> Hi Archit,
>>>
>>> (CC'ing Boris Brezillon)
>>>
>>> Thank you for the patch.
>>>
>>> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>>>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
>>>> the other hand, is going be a normal i2c client device creating bridge
>>>> and connector entities.
>>>
>>> Please, no. It's really time to stop piling hacks and fix the problem
>>> properly. There's no reason to have separate APIs for I2C slave encoders and
>>> DRM bridges. Those two APIs need to be merged, and then you'll find it much
>>> easier to implement ADV7533 support.
>>
>> i2c slave encoders and bridges aren't exactly the same. slave encoders
>> are used when the there is no real 'encoder' in the display chain.
>> bridges are used when there is already an encoder available, and the
>> bridge entity represents another encoder in the chain.
>>
>> ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a
>> crtc for drm drivers.
>>
>> ADV7533 takes in MIPI DSI data, which is generally the output of an
>> encoder for drm drivers.
>>
>> Therefore, having i2c slave encoder for the former and bridge for the
>> latter made sense to me.
>>
>> I do agree that it would be better if they were somehow merged. It
>> would prevent the fragmentation we currently have among encoder
>> chips.
>>
>> One possible way would be to convert slave encoder to bridge. With
>> this, an i2c slave encoder would be a 'dummy encoder' and a bridge.
>> i2c slave encoders even now just tie the slave encoder helper funcs
>> to encoder helper funcs. So it's not really any different.
>>
>> Merging these 2 entities would be nice, but we're still shying away
>> from the larger problem of creating generic encoder chains. The
>> ideal solution would be for bridges and slave encoders to just be
>> 'encoders', and the facility to connect on encoder output to the
>> input of another. I don't know how easy it is to do this, and
>> whether it'll break userspace.
>
> Yes, that's pretty much what I was trying to do.
> I'd also like to ease display pipelines creation by providing helper
> functions, so that display controller don't have to worry about encoders
> and connectors creation if these ones are attached to external encoders.
>
>>
>> Archit
>>
>>>
>>> Boris, I know you were experimenting with that, do you have anything to report
>>> ?
>
> Nope, I didn't work on it since last time we talked about it. I pushed
> my work here if you want to have a look [1].

I went through the branch you shared. From what I understood, the 
encoder chain comprises of one 'real' encoder object (drm_encoder) in
the 'drm_encoder_chain' struct. This drm_encoder encapsulates all the 
'encoder elements' forming the chain.

I'm guessing the various dridge/slave encoder drivers would have to be 
changed to now create a drm_encoder_element object, replacing 
drm_bridge/drm_i2c_slave_encoder objects.

One problem I see with this approach is that we can't use this when
the display controller already exposes a drm_encoder. I.e, it already
creates a drm_encoder object. If we want the encoder chain to be
connected to the output of this encoder, we'll need to link the 2
drm_encoders together, which isn't possible at the moment.

I guess we have two ways to go about this:

1) Have an approach like this where all the entities in the encoder
chain connect to just one encoder. We define the sequence in which
they are connected. The drm kms framework acts as if this chain
doesn't exist, and assumes that this is what the encoder is
outputting.

2) Make each element in the chain be a 'drm_encoder' object in itself. 
This would be a more intrusive change, since drm_encoders are expected 
to receive an input from crtc, and output to a connector. Furthermore, 
it may confuse userspace what encoder to chose.

For 1), we could either work more on your approach, or use drm_bridges. 
drm_bridges can already be chained to each other, and bridge ops of each 
bridge in the chain are called successively. It still relies
on the device drivers to form the chain, which is something your
approach takes care of by itself. But that's something that can be
extended for bridges too.

Laurent,

Merging i2c slave encoders and bridges is possible, but there is no
guarantee that the new solution would be future proof and work well
with encoder chains. We needed more consensus from folks on
dri-devel.

For ADV7533, could you please look at the other parts? Especially the
one that creates a dummy mipi DSI device, and connecting to a dsi
host. I'd previously posted a RFC to enable that:

http://lkml.iu.edu/hypermail/linux/kernel/1506.3/03249.html

Archit

>
> Best Regards,
>
> Boris
>
> [1]https://github.com/bbrezillon/linux-at91/tree/drm-encoder-chain-WIP
>
>
>
Boris BREZILLON July 31, 2015, 9:12 a.m. UTC | #5
Hi Archit,

On Fri, 31 Jul 2015 10:56:20 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> Hi Boris, Laurent,
> 
> On 07/28/2015 08:08 PM, Boris Brezillon wrote:
> > Archit, Laurent,
> >
> > On Tue, 28 Jul 2015 13:47:37 +0530
> > Archit Taneja <architt@codeaurora.org> wrote:
> >
> >> Hi,
> >>
> >> On 07/27/2015 02:29 PM, Laurent Pinchart wrote:
> >>> Hi Archit,
> >>>
> >>> (CC'ing Boris Brezillon)
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
> >>>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
> >>>> the other hand, is going be a normal i2c client device creating bridge
> >>>> and connector entities.
> >>>
> >>> Please, no. It's really time to stop piling hacks and fix the problem
> >>> properly. There's no reason to have separate APIs for I2C slave encoders and
> >>> DRM bridges. Those two APIs need to be merged, and then you'll find it much
> >>> easier to implement ADV7533 support.
> >>
> >> i2c slave encoders and bridges aren't exactly the same. slave encoders
> >> are used when the there is no real 'encoder' in the display chain.
> >> bridges are used when there is already an encoder available, and the
> >> bridge entity represents another encoder in the chain.
> >>
> >> ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a
> >> crtc for drm drivers.
> >>
> >> ADV7533 takes in MIPI DSI data, which is generally the output of an
> >> encoder for drm drivers.
> >>
> >> Therefore, having i2c slave encoder for the former and bridge for the
> >> latter made sense to me.
> >>
> >> I do agree that it would be better if they were somehow merged. It
> >> would prevent the fragmentation we currently have among encoder
> >> chips.
> >>
> >> One possible way would be to convert slave encoder to bridge. With
> >> this, an i2c slave encoder would be a 'dummy encoder' and a bridge.
> >> i2c slave encoders even now just tie the slave encoder helper funcs
> >> to encoder helper funcs. So it's not really any different.
> >>
> >> Merging these 2 entities would be nice, but we're still shying away
> >> from the larger problem of creating generic encoder chains. The
> >> ideal solution would be for bridges and slave encoders to just be
> >> 'encoders', and the facility to connect on encoder output to the
> >> input of another. I don't know how easy it is to do this, and
> >> whether it'll break userspace.
> >
> > Yes, that's pretty much what I was trying to do.
> > I'd also like to ease display pipelines creation by providing helper
> > functions, so that display controller don't have to worry about encoders
> > and connectors creation if these ones are attached to external encoders.
> >
> >>
> >> Archit
> >>
> >>>
> >>> Boris, I know you were experimenting with that, do you have anything to report
> >>> ?
> >
> > Nope, I didn't work on it since last time we talked about it. I pushed
> > my work here if you want to have a look [1].
> 
> I went through the branch you shared. From what I understood, the 
> encoder chain comprises of one 'real' encoder object (drm_encoder) in
> the 'drm_encoder_chain' struct. This drm_encoder encapsulates all the 
> 'encoder elements' forming the chain.
> 
> I'm guessing the various dridge/slave encoder drivers would have to be 
> changed to now create a drm_encoder_element object, replacing 
> drm_bridge/drm_i2c_slave_encoder objects.
> 
> One problem I see with this approach is that we can't use this when
> the display controller already exposes a drm_encoder. I.e, it already
> creates a drm_encoder object. If we want the encoder chain to be
> connected to the output of this encoder, we'll need to link the 2
> drm_encoders together, which isn't possible at the moment.

Actually my goal was to move everybody to the drm_encoder_element model,
even the encoder directly provided by the display controller IP.
If the internal encoder is actually directly connected to a connector,
then the encoder chain will just contain one element, but everything
should work fine.

> 
> I guess we have two ways to go about this:
> 
> 1) Have an approach like this where all the entities in the encoder
> chain connect to just one encoder. We define the sequence in which
> they are connected. The drm kms framework acts as if this chain
> doesn't exist, and assumes that this is what the encoder is
> outputting.

Yep, that's pretty much what I've done. The main reason for doing that
is to keep the interface with the userspace unchanged.

> 
> 2) Make each element in the chain be a 'drm_encoder' object in itself. 
> This would be a more intrusive change, since drm_encoders are expected 
> to receive an input from crtc, and output to a connector. Furthermore, 
> it may confuse userspace what encoder to chose.

That's why Laurent suggested to go for the 1st approach.

> 
> For 1), we could either work more on your approach, or use drm_bridges. 
> drm_bridges can already be chained to each other, and bridge ops of each 
> bridge in the chain are called successively. It still relies
> on the device drivers to form the chain, which is something your
> approach takes care of by itself. But that's something that can be
> extended for bridges too.

Can we really chain several bridges ?

Also note that I plan to automate the encoder chain creation, or at
least provide helper functions so that in standard cases the display
controller does not have to bother creating its encoder chain.
This is particularly true for platforms supporting DT, the encoder
chain + connector can be declared in a generic way, and the core could
provide helper functions to parse and create the encoder chain and the
endpoint connector.

> 
> Laurent,
> 
> Merging i2c slave encoders and bridges is possible, but there is no
> guarantee that the new solution would be future proof and work well
> with encoder chains. We needed more consensus from folks on
> dri-devel.

I'll let Laurent correct me if I'm wrong, but I think the plan was to
move all slave encoders and bridges to the encoder element
representation.

Best Regards,

Boris
Archit Taneja July 31, 2015, 10:38 a.m. UTC | #6
On 07/31/2015 02:42 PM, Boris Brezillon wrote:
> Hi Archit,
>
> On Fri, 31 Jul 2015 10:56:20 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
>
>> Hi Boris, Laurent,
>>
>> On 07/28/2015 08:08 PM, Boris Brezillon wrote:
>>> Archit, Laurent,
>>>
>>> On Tue, 28 Jul 2015 13:47:37 +0530
>>> Archit Taneja <architt@codeaurora.org> wrote:
>>>
>>>> Hi,
>>>>
>>>> On 07/27/2015 02:29 PM, Laurent Pinchart wrote:
>>>>> Hi Archit,
>>>>>
>>>>> (CC'ing Boris Brezillon)
>>>>>
>>>>> Thank you for the patch.
>>>>>
>>>>> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>>>>>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
>>>>>> the other hand, is going be a normal i2c client device creating bridge
>>>>>> and connector entities.
>>>>>
>>>>> Please, no. It's really time to stop piling hacks and fix the problem
>>>>> properly. There's no reason to have separate APIs for I2C slave encoders and
>>>>> DRM bridges. Those two APIs need to be merged, and then you'll find it much
>>>>> easier to implement ADV7533 support.
>>>>
>>>> i2c slave encoders and bridges aren't exactly the same. slave encoders
>>>> are used when the there is no real 'encoder' in the display chain.
>>>> bridges are used when there is already an encoder available, and the
>>>> bridge entity represents another encoder in the chain.
>>>>
>>>> ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a
>>>> crtc for drm drivers.
>>>>
>>>> ADV7533 takes in MIPI DSI data, which is generally the output of an
>>>> encoder for drm drivers.
>>>>
>>>> Therefore, having i2c slave encoder for the former and bridge for the
>>>> latter made sense to me.
>>>>
>>>> I do agree that it would be better if they were somehow merged. It
>>>> would prevent the fragmentation we currently have among encoder
>>>> chips.
>>>>
>>>> One possible way would be to convert slave encoder to bridge. With
>>>> this, an i2c slave encoder would be a 'dummy encoder' and a bridge.
>>>> i2c slave encoders even now just tie the slave encoder helper funcs
>>>> to encoder helper funcs. So it's not really any different.
>>>>
>>>> Merging these 2 entities would be nice, but we're still shying away
>>>> from the larger problem of creating generic encoder chains. The
>>>> ideal solution would be for bridges and slave encoders to just be
>>>> 'encoders', and the facility to connect on encoder output to the
>>>> input of another. I don't know how easy it is to do this, and
>>>> whether it'll break userspace.
>>>
>>> Yes, that's pretty much what I was trying to do.
>>> I'd also like to ease display pipelines creation by providing helper
>>> functions, so that display controller don't have to worry about encoders
>>> and connectors creation if these ones are attached to external encoders.
>>>
>>>>
>>>> Archit
>>>>
>>>>>
>>>>> Boris, I know you were experimenting with that, do you have anything to report
>>>>> ?
>>>
>>> Nope, I didn't work on it since last time we talked about it. I pushed
>>> my work here if you want to have a look [1].
>>
>> I went through the branch you shared. From what I understood, the
>> encoder chain comprises of one 'real' encoder object (drm_encoder) in
>> the 'drm_encoder_chain' struct. This drm_encoder encapsulates all the
>> 'encoder elements' forming the chain.
>>
>> I'm guessing the various dridge/slave encoder drivers would have to be
>> changed to now create a drm_encoder_element object, replacing
>> drm_bridge/drm_i2c_slave_encoder objects.
>>
>> One problem I see with this approach is that we can't use this when
>> the display controller already exposes a drm_encoder. I.e, it already
>> creates a drm_encoder object. If we want the encoder chain to be
>> connected to the output of this encoder, we'll need to link the 2
>> drm_encoders together, which isn't possible at the moment.
>
> Actually my goal was to move everybody to the drm_encoder_element model,
> even the encoder directly provided by the display controller IP.
> If the internal encoder is actually directly connected to a connector,
> then the encoder chain will just contain one element, but everything
> should work fine.

Okay. That approach makes sense.

It might be good to have a look at the current state of drm_bridge. We 
need to probably make a call between extending bridges or starting with
encoder elements from scratch. Extending bridges might be less 
intrusive. Although, encoder elements is more uniform. In bridges,
we'll be stuck with two entities: One encoder (drm_encoder), followed by 
a chain of bridges.

>
>>
>> I guess we have two ways to go about this:
>>
>> 1) Have an approach like this where all the entities in the encoder
>> chain connect to just one encoder. We define the sequence in which
>> they are connected. The drm kms framework acts as if this chain
>> doesn't exist, and assumes that this is what the encoder is
>> outputting.
>
> Yep, that's pretty much what I've done. The main reason for doing that
> is to keep the interface with the userspace unchanged.
>
>>
>> 2) Make each element in the chain be a 'drm_encoder' object in itself.
>> This would be a more intrusive change, since drm_encoders are expected
>> to receive an input from crtc, and output to a connector. Furthermore,
>> it may confuse userspace what encoder to chose.
>
> That's why Laurent suggested to go for the 1st approach.
>
>>
>> For 1), we could either work more on your approach, or use drm_bridges.
>> drm_bridges can already be chained to each other, and bridge ops of each
>> bridge in the chain are called successively. It still relies
>> on the device drivers to form the chain, which is something your
>> approach takes care of by itself. But that's something that can be
>> extended for bridges too.
>
> Can we really chain several bridges ?

Yes. The support was recently added. We can link one bridge to another
via drm_bridge_attach(), by populating the bridge->next field.

The bridge helpers used in atomic_helper.c and crtc_helper.c
recursively call all the bridges in the chain.

>
> Also note that I plan to automate the encoder chain creation, or at
> least provide helper functions so that in standard cases the display
> controller does not have to bother creating its encoder chain.
> This is particularly true for platforms supporting DT, the encoder
> chain + connector can be declared in a generic way, and the core could
> provide helper functions to parse and create the encoder chain and the
> endpoint connector.

This would be quite useful.

>
>>
>> Laurent,
>>
>> Merging i2c slave encoders and bridges is possible, but there is no
>> guarantee that the new solution would be future proof and work well
>> with encoder chains. We needed more consensus from folks on
>> dri-devel.
>
> I'll let Laurent correct me if I'm wrong, but I think the plan was to
> move all slave encoders and bridges to the encoder element
> representation.

Some troubles I've had when working with encoder chains:

- There can be dependencies between two elements in the chain. For
example, an element in the chain might provide interface clocks for the
next element in the chain. The laterelement shouldn't even try to touch
its registers if the previous element isn't enabled.

- The sequence in which each encoder element needs to be called. Some
have to be first to last, others last to first.

- Some external encoder drivers(currently bridge, i2c slaves) create
their own connectors within the driver. If such an encoder is placed
in the middle of an encoder chain. It could lead to problems.

We might want to consider these (and probably more things) when working
on the solution.

Thanks,
Archit
Rob Clark July 31, 2015, 12:13 p.m. UTC | #7
On Fri, Jul 31, 2015 at 5:12 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Archit,
>
> On Fri, 31 Jul 2015 10:56:20 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
>
>> Hi Boris, Laurent,
>>
>> On 07/28/2015 08:08 PM, Boris Brezillon wrote:
>> > Archit, Laurent,
>> >
>> > On Tue, 28 Jul 2015 13:47:37 +0530
>> > Archit Taneja <architt@codeaurora.org> wrote:
>> >
>> >> Hi,
>> >>
>> >> On 07/27/2015 02:29 PM, Laurent Pinchart wrote:
>> >>> Hi Archit,
>> >>>
>> >>> (CC'ing Boris Brezillon)
>> >>>
>> >>> Thank you for the patch.
>> >>>
>> >>> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>> >>>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
>> >>>> the other hand, is going be a normal i2c client device creating bridge
>> >>>> and connector entities.
>> >>>
>> >>> Please, no. It's really time to stop piling hacks and fix the problem
>> >>> properly. There's no reason to have separate APIs for I2C slave encoders and
>> >>> DRM bridges. Those two APIs need to be merged, and then you'll find it much
>> >>> easier to implement ADV7533 support.
>> >>
>> >> i2c slave encoders and bridges aren't exactly the same. slave encoders
>> >> are used when the there is no real 'encoder' in the display chain.
>> >> bridges are used when there is already an encoder available, and the
>> >> bridge entity represents another encoder in the chain.
>> >>
>> >> ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a
>> >> crtc for drm drivers.
>> >>
>> >> ADV7533 takes in MIPI DSI data, which is generally the output of an
>> >> encoder for drm drivers.
>> >>
>> >> Therefore, having i2c slave encoder for the former and bridge for the
>> >> latter made sense to me.
>> >>
>> >> I do agree that it would be better if they were somehow merged. It
>> >> would prevent the fragmentation we currently have among encoder
>> >> chips.
>> >>
>> >> One possible way would be to convert slave encoder to bridge. With
>> >> this, an i2c slave encoder would be a 'dummy encoder' and a bridge.
>> >> i2c slave encoders even now just tie the slave encoder helper funcs
>> >> to encoder helper funcs. So it's not really any different.
>> >>
>> >> Merging these 2 entities would be nice, but we're still shying away
>> >> from the larger problem of creating generic encoder chains. The
>> >> ideal solution would be for bridges and slave encoders to just be
>> >> 'encoders', and the facility to connect on encoder output to the
>> >> input of another. I don't know how easy it is to do this, and
>> >> whether it'll break userspace.
>> >
>> > Yes, that's pretty much what I was trying to do.
>> > I'd also like to ease display pipelines creation by providing helper
>> > functions, so that display controller don't have to worry about encoders
>> > and connectors creation if these ones are attached to external encoders.
>> >
>> >>
>> >> Archit
>> >>
>> >>>
>> >>> Boris, I know you were experimenting with that, do you have anything to report
>> >>> ?
>> >
>> > Nope, I didn't work on it since last time we talked about it. I pushed
>> > my work here if you want to have a look [1].
>>
>> I went through the branch you shared. From what I understood, the
>> encoder chain comprises of one 'real' encoder object (drm_encoder) in
>> the 'drm_encoder_chain' struct. This drm_encoder encapsulates all the
>> 'encoder elements' forming the chain.
>>
>> I'm guessing the various dridge/slave encoder drivers would have to be
>> changed to now create a drm_encoder_element object, replacing
>> drm_bridge/drm_i2c_slave_encoder objects.
>>
>> One problem I see with this approach is that we can't use this when
>> the display controller already exposes a drm_encoder. I.e, it already
>> creates a drm_encoder object. If we want the encoder chain to be
>> connected to the output of this encoder, we'll need to link the 2
>> drm_encoders together, which isn't possible at the moment.
>
> Actually my goal was to move everybody to the drm_encoder_element model,
> even the encoder directly provided by the display controller IP.
> If the internal encoder is actually directly connected to a connector,
> then the encoder chain will just contain one element, but everything
> should work fine.

so..  I'd be careful about changing the role of drm_encoder, as it
does play a small role in the interface exposed to userspace.

If you do anything, I think you need to make i2c-encoder-slave stuff
look like a drm_bridge + drm_connector, since bridge is not visible to
userspace and can already be chained... which I think ends up making
it more like how adv7533 looks w/ archit's patches.

That said, the adv7511 type case (raw parallel output) is kind of a
better fit for the existing i2c-slave-encoder model, vs adv7533 where
you already have a dsi encoder and is a better fit for the drm_bridge
model.  So maybe there is still a place for both.

At any rate, if we do unify, I think we should go towards the
drm_bridge + drm_connector approach and migrate i2c-encoder users over
to that.  Which would make Archit's approach a reasonable transition
step.  We just drop the i2c-encoder part of it when none of the
adv7511 users still depend on that.

BR,
-R


>>
>> I guess we have two ways to go about this:
>>
>> 1) Have an approach like this where all the entities in the encoder
>> chain connect to just one encoder. We define the sequence in which
>> they are connected. The drm kms framework acts as if this chain
>> doesn't exist, and assumes that this is what the encoder is
>> outputting.
>
> Yep, that's pretty much what I've done. The main reason for doing that
> is to keep the interface with the userspace unchanged.
>
>>
>> 2) Make each element in the chain be a 'drm_encoder' object in itself.
>> This would be a more intrusive change, since drm_encoders are expected
>> to receive an input from crtc, and output to a connector. Furthermore,
>> it may confuse userspace what encoder to chose.
>
> That's why Laurent suggested to go for the 1st approach.
>
>>
>> For 1), we could either work more on your approach, or use drm_bridges.
>> drm_bridges can already be chained to each other, and bridge ops of each
>> bridge in the chain are called successively. It still relies
>> on the device drivers to form the chain, which is something your
>> approach takes care of by itself. But that's something that can be
>> extended for bridges too.
>
> Can we really chain several bridges ?
>
> Also note that I plan to automate the encoder chain creation, or at
> least provide helper functions so that in standard cases the display
> controller does not have to bother creating its encoder chain.
> This is particularly true for platforms supporting DT, the encoder
> chain + connector can be declared in a generic way, and the core could
> provide helper functions to parse and create the encoder chain and the
> endpoint connector.
>
>>
>> Laurent,
>>
>> Merging i2c slave encoders and bridges is possible, but there is no
>> guarantee that the new solution would be future proof and work well
>> with encoder chains. We needed more consensus from folks on
>> dri-devel.
>
> I'll let Laurent correct me if I'm wrong, but I think the plan was to
> move all slave encoders and bridges to the encoder element
> representation.
>
> Best Regards,
>
> Boris
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Boris BREZILLON July 31, 2015, 12:58 p.m. UTC | #8
Hi Rob,

On Fri, 31 Jul 2015 08:13:59 -0400
Rob Clark <robdclark@gmail.com> wrote:

> >>
> >> I went through the branch you shared. From what I understood, the
> >> encoder chain comprises of one 'real' encoder object (drm_encoder) in
> >> the 'drm_encoder_chain' struct. This drm_encoder encapsulates all the
> >> 'encoder elements' forming the chain.
> >>
> >> I'm guessing the various dridge/slave encoder drivers would have to be
> >> changed to now create a drm_encoder_element object, replacing
> >> drm_bridge/drm_i2c_slave_encoder objects.
> >>
> >> One problem I see with this approach is that we can't use this when
> >> the display controller already exposes a drm_encoder. I.e, it already
> >> creates a drm_encoder object. If we want the encoder chain to be
> >> connected to the output of this encoder, we'll need to link the 2
> >> drm_encoders together, which isn't possible at the moment.
> >
> > Actually my goal was to move everybody to the drm_encoder_element model,
> > even the encoder directly provided by the display controller IP.
> > If the internal encoder is actually directly connected to a connector,
> > then the encoder chain will just contain one element, but everything
> > should work fine.
> 
> so..  I'd be careful about changing the role of drm_encoder, as it
> does play a small role in the interface exposed to userspace.

I don't think I changed the role of the drm_encoder in my approach.
Actually I'm only exposing one encoder for the whole encoder chain.
The encoder chain is containing one or several encoder elements, which
are not exposed to userspace.

> 
> If you do anything, I think you need to make i2c-encoder-slave stuff
> look like a drm_bridge + drm_connector, since bridge is not visible to
> userspace and can already be chained... which I think ends up making
> it more like how adv7533 looks w/ archit's patches.

Okay, maybe we can do the same with drm bridges (I wasn't aware that
these ones could be chained before Archit mentioned it).
I remember that I was missing a few features in the DRM bridge
implementation (like the possibility to negotiate the format passed on
the link between 2 encoders), but that's probably something we can add.

> 
> That said, the adv7511 type case (raw parallel output) is kind of a
> better fit for the existing i2c-slave-encoder model, vs adv7533 where
> you already have a dsi encoder and is a better fit for the drm_bridge
> model.  So maybe there is still a place for both.

Excuse my ignorance, but I still don't get why the RGB/MIPI DPI are
not represented as encoders. They are dummy encoders which just
forwards the output of the display controller in raw RGB format, but
still. IMHO, representing them as encoders in the chain would ease the
whole thing.
Moreover, by doing that we would be able to link this RGB/DPI encoder to
a bridge, and we wouldn't have to differentiate the encoder-slave and
bridge elements.

> 
> At any rate, if we do unify, I think we should go towards the
> drm_bridge + drm_connector approach and migrate i2c-encoder users over
> to that.  Which would make Archit's approach a reasonable transition
> step.  We just drop the i2c-encoder part of it when none of the
> adv7511 users still depend on that.

Another problem I've seen with some drm bridge drivers is that they
directly create their own connector, which, as Archit stated, is wrong
if you decide to chain this bridge with another bridge. The other
reason why the bridge should not create the connector by its own is
that in some case the encoder supports different types of connectors (a
TDMS encoder can be used to output HDMI or DVI), and thus, selecting
the connector type should be left to the part responsible for creating
the display pipelines.

This being said, I'm definitely not an expert in this area, so don't
hesitate to show me where I'm wrong.

Best Regards,

Boris
Rob Clark July 31, 2015, 2:48 p.m. UTC | #9
On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Rob,
>
> On Fri, 31 Jul 2015 08:13:59 -0400
> Rob Clark <robdclark@gmail.com> wrote:
>
>> >>
>> >> I went through the branch you shared. From what I understood, the
>> >> encoder chain comprises of one 'real' encoder object (drm_encoder) in
>> >> the 'drm_encoder_chain' struct. This drm_encoder encapsulates all the
>> >> 'encoder elements' forming the chain.
>> >>
>> >> I'm guessing the various dridge/slave encoder drivers would have to be
>> >> changed to now create a drm_encoder_element object, replacing
>> >> drm_bridge/drm_i2c_slave_encoder objects.
>> >>
>> >> One problem I see with this approach is that we can't use this when
>> >> the display controller already exposes a drm_encoder. I.e, it already
>> >> creates a drm_encoder object. If we want the encoder chain to be
>> >> connected to the output of this encoder, we'll need to link the 2
>> >> drm_encoders together, which isn't possible at the moment.
>> >
>> > Actually my goal was to move everybody to the drm_encoder_element model,
>> > even the encoder directly provided by the display controller IP.
>> > If the internal encoder is actually directly connected to a connector,
>> > then the encoder chain will just contain one element, but everything
>> > should work fine.
>>
>> so..  I'd be careful about changing the role of drm_encoder, as it
>> does play a small role in the interface exposed to userspace.
>
> I don't think I changed the role of the drm_encoder in my approach.
> Actually I'm only exposing one encoder for the whole encoder chain.
> The encoder chain is containing one or several encoder elements, which
> are not exposed to userspace.
>
>>
>> If you do anything, I think you need to make i2c-encoder-slave stuff
>> look like a drm_bridge + drm_connector, since bridge is not visible to
>> userspace and can already be chained... which I think ends up making
>> it more like how adv7533 looks w/ archit's patches.
>
> Okay, maybe we can do the same with drm bridges (I wasn't aware that
> these ones could be chained before Archit mentioned it).
> I remember that I was missing a few features in the DRM bridge
> implementation (like the possibility to negotiate the format passed on
> the link between 2 encoders), but that's probably something we can add.

chaining bridges is very recent addition, fwiw

At any rate, extending bridge where needed seems preferable over
adding new types of objects, I think.

>>
>> That said, the adv7511 type case (raw parallel output) is kind of a
>> better fit for the existing i2c-slave-encoder model, vs adv7533 where
>> you already have a dsi encoder and is a better fit for the drm_bridge
>> model.  So maybe there is still a place for both.
>
> Excuse my ignorance, but I still don't get why the RGB/MIPI DPI are
> not represented as encoders. They are dummy encoders which just
> forwards the output of the display controller in raw RGB format, but
> still. IMHO, representing them as encoders in the chain would ease the
> whole thing.

Yeah, creating a dummy encoder in the driver would be the approach to
switch from i2c-encoder to bridge.  I didn't mean to imply that this
couldn't be done.  The only point I was trying to make was that for
simple cases don't need this currently.  (The case I'm more familiar
w/ is tilcdc -> tda998x but I guess other simple display controllers
to adv7511 is probably similar)

I guess in the i2c-encoder case the driver ends up creating a sort of
shim encoder/connector, so maybe it isn't that much different.  It is
a bunch of shuffling things around to change from i2c-encoder to
bridge, and it ends up effecting a bunch of drivers (including some
less simple ones like nouveau), so I'm not sure the best migration
path.  Exposing both i2c-encoder and bridge interfaces for a period of
time seems unavoidable..

> Moreover, by doing that we would be able to link this RGB/DPI encoder to
> a bridge, and we wouldn't have to differentiate the encoder-slave and
> bridge elements.
>
>>
>> At any rate, if we do unify, I think we should go towards the
>> drm_bridge + drm_connector approach and migrate i2c-encoder users over
>> to that.  Which would make Archit's approach a reasonable transition
>> step.  We just drop the i2c-encoder part of it when none of the
>> adv7511 users still depend on that.
>
> Another problem I've seen with some drm bridge drivers is that they
> directly create their own connector, which, as Archit stated, is wrong
> if you decide to chain this bridge with another bridge. The other

I agree with Archit on this.  He took this approach w/ msm support for
external bridges, and it seems sensible that the last bridge
constructs the connector.  (Plus presumably that is where hpd, ddc
probing, etc, is happing)

> reason why the bridge should not create the connector by its own is
> that in some case the encoder supports different types of connectors (a
> TDMS encoder can be used to output HDMI or DVI), and thus, selecting
> the connector type should be left to the part responsible for creating
> the display pipelines.

did you mean "should" instead of "should not"?  Otherwise I don't
think I understand..

BR,
-R

> This being said, I'm definitely not an expert in this area, so don't
> hesitate to show me where I'm wrong.
>
> Best Regards,
>
> Boris
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Andrzej Hajda Aug. 3, 2015, 12:03 p.m. UTC | #10
Hi,

On 07/31/2015 04:48 PM, Rob Clark wrote:
> On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
>> Hi Rob,
>>
>> On Fri, 31 Jul 2015 08:13:59 -0400
>> Rob Clark <robdclark@gmail.com> wrote:
>>

(...)

>>
>> Another problem I've seen with some drm bridge drivers is that they
>> directly create their own connector, which, as Archit stated, is wrong
>> if you decide to chain this bridge with another bridge. The other
> 
> I agree with Archit on this.  He took this approach w/ msm support for
> external bridges, and it seems sensible that the last bridge
> constructs the connector.  (Plus presumably that is where hpd, ddc
> probing, etc, is happing)

With this approach many bridges should construct connector conditionally,
depending if there is something behind them in pipeline (the same is true for
encoders and even crtcs). It works, but for me there is lot of unnecessary code
and all those conditional paths make things less friendly for development.
Wouldn't be better to move creation of the connector to the main drm driver,
it would require probably adding some opses/fields to drm_bridges but the
drivers would be simpler, I guess.


Regards
Andrzej

> 
>> reason why the bridge should not create the connector by its own is
>> that in some case the encoder supports different types of connectors (a
>> TDMS encoder can be used to output HDMI or DVI), and thus, selecting
>> the connector type should be left to the part responsible for creating
>> the display pipelines.
> 
> did you mean "should" instead of "should not"?  Otherwise I don't
> think I understand..
> 
> BR,
> -R
> 
>> This being said, I'm definitely not an expert in this area, so don't
>> hesitate to show me where I'm wrong.
>>
>> Best Regards,
>>
>> Boris
>>
>> --
>> Boris Brezillon, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
Rob Clark Aug. 3, 2015, 2:04 p.m. UTC | #11
On Mon, Aug 3, 2015 at 8:03 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> Hi,
>
> On 07/31/2015 04:48 PM, Rob Clark wrote:
>> On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>>> Hi Rob,
>>>
>>> On Fri, 31 Jul 2015 08:13:59 -0400
>>> Rob Clark <robdclark@gmail.com> wrote:
>>>
>
> (...)
>
>>>
>>> Another problem I've seen with some drm bridge drivers is that they
>>> directly create their own connector, which, as Archit stated, is wrong
>>> if you decide to chain this bridge with another bridge. The other
>>
>> I agree with Archit on this.  He took this approach w/ msm support for
>> external bridges, and it seems sensible that the last bridge
>> constructs the connector.  (Plus presumably that is where hpd, ddc
>> probing, etc, is happing)
>
> With this approach many bridges should construct connector conditionally,
> depending if there is something behind them in pipeline (the same is true for
> encoders and even crtcs). It works, but for me there is lot of unnecessary code
> and all those conditional paths make things less friendly for development.
> Wouldn't be better to move creation of the connector to the main drm driver,
> it would require probably adding some opses/fields to drm_bridges but the
> drivers would be simpler, I guess.

six of one, half dozen of the other..   you'd still need to implement
the hpd and ddc probe bits, and that sort of thing *somewhere*.
Whether it is directly by implementing drm_connector in the bridge, or
indirectly via some extra drm_bridge op's called by a shim connector
in the main drm driver, doesn't really seem to change things..

BR,
-R

>
> Regards
> Andrzej
>
>>
>>> reason why the bridge should not create the connector by its own is
>>> that in some case the encoder supports different types of connectors (a
>>> TDMS encoder can be used to output HDMI or DVI), and thus, selecting
>>> the connector type should be left to the part responsible for creating
>>> the display pipelines.
>>
>> did you mean "should" instead of "should not"?  Otherwise I don't
>> think I understand..
>>
>> BR,
>> -R
>>
>>> This being said, I'm definitely not an expert in this area, so don't
>>> hesitate to show me where I'm wrong.
>>>
>>> Best Regards,
>>>
>>> Boris
>>>
>>> --
>>> Boris Brezillon, Free Electrons
>>> Embedded Linux and Kernel engineering
>>> http://free-electrons.com
>
Andrzej Hajda Aug. 4, 2015, 5:16 a.m. UTC | #12
On 08/03/2015 04:04 PM, Rob Clark wrote:
> On Mon, Aug 3, 2015 at 8:03 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> Hi,
>>
>> On 07/31/2015 04:48 PM, Rob Clark wrote:
>>> On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon
>>> <boris.brezillon@free-electrons.com> wrote:
>>>> Hi Rob,
>>>>
>>>> On Fri, 31 Jul 2015 08:13:59 -0400
>>>> Rob Clark <robdclark@gmail.com> wrote:
>>>>
>> (...)
>>
>>>> Another problem I've seen with some drm bridge drivers is that they
>>>> directly create their own connector, which, as Archit stated, is wrong
>>>> if you decide to chain this bridge with another bridge. The other
>>> I agree with Archit on this.  He took this approach w/ msm support for
>>> external bridges, and it seems sensible that the last bridge
>>> constructs the connector.  (Plus presumably that is where hpd, ddc
>>> probing, etc, is happing)
>> With this approach many bridges should construct connector conditionally,
>> depending if there is something behind them in pipeline (the same is true for
>> encoders and even crtcs). It works, but for me there is lot of unnecessary code
>> and all those conditional paths make things less friendly for development.
>> Wouldn't be better to move creation of the connector to the main drm driver,
>> it would require probably adding some opses/fields to drm_bridges but the
>> drivers would be simpler, I guess.
> six of one, half dozen of the other..   you'd still need to implement
> the hpd and ddc probe bits, and that sort of thing *somewhere*.
> Whether it is directly by implementing drm_connector in the bridge, or
> indirectly via some extra drm_bridge op's called by a shim connector
> in the main drm driver, doesn't really seem to change things..

The difference is that instead of duplicating connector related code in every
driver you will call one helper from the main drm driver/core.

Regards
Andrzej

>
> BR,
> -R
>
>> Regards
>> Andrzej
>>
>>>> reason why the bridge should not create the connector by its own is
>>>> that in some case the encoder supports different types of connectors (a
>>>> TDMS encoder can be used to output HDMI or DVI), and thus, selecting
>>>> the connector type should be left to the part responsible for creating
>>>> the display pipelines.
>>> did you mean "should" instead of "should not"?  Otherwise I don't
>>> think I understand..
>>>
>>> BR,
>>> -R
>>>
>>>> This being said, I'm definitely not an expert in this area, so don't
>>>> hesitate to show me where I'm wrong.
>>>>
>>>> Best Regards,
>>>>
>>>> Boris
>>>>
>>>> --
>>>> Boris Brezillon, Free Electrons
>>>> Embedded Linux and Kernel engineering
>>>> http://free-electrons.com
Rob Clark Aug. 4, 2015, 12:24 p.m. UTC | #13
On Tue, Aug 4, 2015 at 1:16 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 08/03/2015 04:04 PM, Rob Clark wrote:
>> On Mon, Aug 3, 2015 at 8:03 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> Hi,
>>>
>>> On 07/31/2015 04:48 PM, Rob Clark wrote:
>>>> On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon
>>>> <boris.brezillon@free-electrons.com> wrote:
>>>>> Hi Rob,
>>>>>
>>>>> On Fri, 31 Jul 2015 08:13:59 -0400
>>>>> Rob Clark <robdclark@gmail.com> wrote:
>>>>>
>>> (...)
>>>
>>>>> Another problem I've seen with some drm bridge drivers is that they
>>>>> directly create their own connector, which, as Archit stated, is wrong
>>>>> if you decide to chain this bridge with another bridge. The other
>>>> I agree with Archit on this.  He took this approach w/ msm support for
>>>> external bridges, and it seems sensible that the last bridge
>>>> constructs the connector.  (Plus presumably that is where hpd, ddc
>>>> probing, etc, is happing)
>>> With this approach many bridges should construct connector conditionally,
>>> depending if there is something behind them in pipeline (the same is true for
>>> encoders and even crtcs). It works, but for me there is lot of unnecessary code
>>> and all those conditional paths make things less friendly for development.
>>> Wouldn't be better to move creation of the connector to the main drm driver,
>>> it would require probably adding some opses/fields to drm_bridges but the
>>> drivers would be simpler, I guess.
>> six of one, half dozen of the other..   you'd still need to implement
>> the hpd and ddc probe bits, and that sort of thing *somewhere*.
>> Whether it is directly by implementing drm_connector in the bridge, or
>> indirectly via some extra drm_bridge op's called by a shim connector
>> in the main drm driver, doesn't really seem to change things..
>
> The difference is that instead of duplicating connector related code in every
> driver you will call one helper from the main drm driver/core.

Which isn't more than a few lines of code.. I mean, looking at a
couple connectors, the bulk of the code is hpd and ddc related.  That
doesn't magically go away.  There isn't that many lines of
boilerplate, so meh.

BR,
-R

> Regards
> Andrzej
>
>>
>> BR,
>> -R
>>
>>> Regards
>>> Andrzej
>>>
>>>>> reason why the bridge should not create the connector by its own is
>>>>> that in some case the encoder supports different types of connectors (a
>>>>> TDMS encoder can be used to output HDMI or DVI), and thus, selecting
>>>>> the connector type should be left to the part responsible for creating
>>>>> the display pipelines.
>>>> did you mean "should" instead of "should not"?  Otherwise I don't
>>>> think I understand..
>>>>
>>>> BR,
>>>> -R
>>>>
>>>>> This being said, I'm definitely not an expert in this area, so don't
>>>>> hesitate to show me where I'm wrong.
>>>>>
>>>>> Best Regards,
>>>>>
>>>>> Boris
>>>>>
>>>>> --
>>>>> Boris Brezillon, Free Electrons
>>>>> Embedded Linux and Kernel engineering
>>>>> http://free-electrons.com
>
Archit Taneja Sept. 2, 2015, 6:30 a.m. UTC | #14
On 08/04/2015 05:54 PM, Rob Clark wrote:
> On Tue, Aug 4, 2015 at 1:16 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 08/03/2015 04:04 PM, Rob Clark wrote:
>>> On Mon, Aug 3, 2015 at 8:03 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> Hi,
>>>>
>>>> On 07/31/2015 04:48 PM, Rob Clark wrote:
>>>>> On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon
>>>>> <boris.brezillon@free-electrons.com> wrote:
>>>>>> Hi Rob,
>>>>>>
>>>>>> On Fri, 31 Jul 2015 08:13:59 -0400
>>>>>> Rob Clark <robdclark@gmail.com> wrote:
>>>>>>
>>>> (...)
>>>>
>>>>>> Another problem I've seen with some drm bridge drivers is that they
>>>>>> directly create their own connector, which, as Archit stated, is wrong
>>>>>> if you decide to chain this bridge with another bridge. The other
>>>>> I agree with Archit on this.  He took this approach w/ msm support for
>>>>> external bridges, and it seems sensible that the last bridge
>>>>> constructs the connector.  (Plus presumably that is where hpd, ddc
>>>>> probing, etc, is happing)
>>>> With this approach many bridges should construct connector conditionally,
>>>> depending if there is something behind them in pipeline (the same is true for
>>>> encoders and even crtcs). It works, but for me there is lot of unnecessary code
>>>> and all those conditional paths make things less friendly for development.
>>>> Wouldn't be better to move creation of the connector to the main drm driver,
>>>> it would require probably adding some opses/fields to drm_bridges but the
>>>> drivers would be simpler, I guess.
>>> six of one, half dozen of the other..   you'd still need to implement
>>> the hpd and ddc probe bits, and that sort of thing *somewhere*.
>>> Whether it is directly by implementing drm_connector in the bridge, or
>>> indirectly via some extra drm_bridge op's called by a shim connector
>>> in the main drm driver, doesn't really seem to change things..
>>
>> The difference is that instead of duplicating connector related code in every
>> driver you will call one helper from the main drm driver/core.
>
> Which isn't more than a few lines of code.. I mean, looking at a
> couple connectors, the bulk of the code is hpd and ddc related.  That
> doesn't magically go away.  There isn't that many lines of
> boilerplate, so meh.

Could we get to a consensus for this?

Is it okay for bridge and i2c_encoder to co-exist for now?

Thanks,
Archit

>
> BR,
> -R
>
>> Regards
>> Andrzej
>>
>>>
>>> BR,
>>> -R
>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>>> reason why the bridge should not create the connector by its own is
>>>>>> that in some case the encoder supports different types of connectors (a
>>>>>> TDMS encoder can be used to output HDMI or DVI), and thus, selecting
>>>>>> the connector type should be left to the part responsible for creating
>>>>>> the display pipelines.
>>>>> did you mean "should" instead of "should not"?  Otherwise I don't
>>>>> think I understand..
>>>>>
>>>>> BR,
>>>>> -R
>>>>>
>>>>>> This being said, I'm definitely not an expert in this area, so don't
>>>>>> hesitate to show me where I'm wrong.
>>>>>>
>>>>>> Best Regards,
>>>>>>
>>>>>> Boris
>>>>>>
>>>>>> --
>>>>>> Boris Brezillon, Free Electrons
>>>>>> Embedded Linux and Kernel engineering
>>>>>> http://free-electrons.com
>>
Rob Clark Dec. 3, 2015, 3:02 p.m. UTC | #15
On Mon, Jul 27, 2015 at 4:59 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Archit,
>
> (CC'ing Boris Brezillon)
>
> Thank you for the patch.
>
> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
>> the other hand, is going be a normal i2c client device creating bridge
>> and connector entities.
>
> Please, no. It's really time to stop piling hacks and fix the problem
> properly. There's no reason to have separate APIs for I2C slave encoders and
> DRM bridges. Those two APIs need to be merged, and then you'll find it much
> easier to implement ADV7533 support.

btw, what is the status here?  I'd kind of lost track of the
discussion, but I'm getting impatient that it is somehow taking
ridiculously long to get adv7533 support merged.  (It's good thing the
x86/desktop folks don't bikeshed so much..  I'd hate to wait a year
for my new laptop to be supported..)

Anyways, if needed, just copy/paste the adv7511/7533 code into a
separate bridge-only driver, and we'll use that.  Once the
i2c-slave-encoder users for adv7511 are converted over, we can delete
the original slave encoder driver.  That seems like a sane transition
plan to a bridge-only world.

BR,
-R

> Boris, I know you were experimenting with that, do you have anything to report
> ?
>
>> Move the code in encoder slave functions to generate helpers that are
>> agnostic to the drm object type. These helpers will later also be used
>> by bridge and connecter helper functions.
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>  drivers/gpu/drm/i2c/adv7511.c | 80 +++++++++++++++++++++++++++------------
>>  1 file changed, 57 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
>> index 63a3d20..46fb24d 100644
>> --- a/drivers/gpu/drm/i2c/adv7511.c
>> +++ b/drivers/gpu/drm/i2c/adv7511.c
>> @@ -612,13 +612,11 @@ static int adv7511_get_edid_block(void *data, u8 *buf,
>> unsigned int block, }
>>
>>  /*
>> ---------------------------------------------------------------------------
>> -- - * Encoder operations
>> + * ADV75xx helpers
>>   */
>> -
>> -static int adv7511_get_modes(struct drm_encoder *encoder,
>> -                          struct drm_connector *connector)
>> +static int adv7511_get_modes(struct adv7511 *adv7511,
>> +             struct drm_connector *connector)
>>  {
>> -     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>       struct edid *edid;
>>       unsigned int count;
>>
>> @@ -656,21 +654,10 @@ static int adv7511_get_modes(struct drm_encoder
>> *encoder, return count;
>>  }
>>
>> -static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
>> -{
>> -     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> -
>> -     if (mode == DRM_MODE_DPMS_ON)
>> -             adv7511_power_on(adv7511);
>> -     else
>> -             adv7511_power_off(adv7511);
>> -}
>> -
>>  static enum drm_connector_status
>> -adv7511_encoder_detect(struct drm_encoder *encoder,
>> +adv7511_detect(struct adv7511 *adv7511,
>>                      struct drm_connector *connector)
>>  {
>> -     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>       enum drm_connector_status status;
>>       unsigned int val;
>>       bool hpd;
>> @@ -694,7 +681,7 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
>>       if (status == connector_status_connected && hpd && adv7511->powered) {
>>               regcache_mark_dirty(adv7511->regmap);
>>               adv7511_power_on(adv7511);
>> -             adv7511_get_modes(encoder, connector);
>> +             adv7511_get_modes(adv7511, connector);
>>               if (adv7511->status == connector_status_connected)
>>                       status = connector_status_disconnected;
>>       } else {
>> @@ -708,8 +695,8 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
>>       return status;
>>  }
>>
>> -static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
>> -                                   struct drm_display_mode *mode)
>> +static int adv7511_mode_valid(struct adv7511 *adv7511,
>> +                                  const struct drm_display_mode *mode)
>>  {
>>       if (mode->clock > 165000)
>>               return MODE_CLOCK_HIGH;
>> @@ -717,11 +704,10 @@ static int adv7511_encoder_mode_valid(struct
>> drm_encoder *encoder, return MODE_OK;
>>  }
>>
>> -static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
>> +static void adv7511_mode_set(struct adv7511 *adv7511,
>>                                    struct drm_display_mode *mode,
>>                                    struct drm_display_mode *adj_mode)
>>  {
>> -     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>>       unsigned int low_refresh_rate;
>>       unsigned int hsync_polarity = 0;
>>       unsigned int vsync_polarity = 0;
>> @@ -812,12 +798,60 @@ static void adv7511_encoder_mode_set(struct
>> drm_encoder *encoder, adv7511->f_tmds = mode->clock;
>>  }
>>
>> +/*
>> ---------------------------------------------------------------------------
>> -- + * Encoder operations
>> + */
>> +
>> +static int adv7511_encoder_get_modes(struct drm_encoder *encoder,
>> +                          struct drm_connector *connector)
>> +{
>> +     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +     return adv7511_get_modes(adv7511, connector);
>> +}
>> +
>> +static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
>> +{
>> +     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +     if (mode == DRM_MODE_DPMS_ON)
>> +             adv7511_power_on(adv7511);
>> +     else
>> +             adv7511_power_off(adv7511);
>> +}
>> +
>> +static enum drm_connector_status
>> +adv7511_encoder_detect(struct drm_encoder *encoder,
>> +                    struct drm_connector *connector)
>> +{
>> +     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +     return adv7511_detect(adv7511, connector);
>> +}
>> +
>> +static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
>> +                                   struct drm_display_mode *mode)
>> +{
>> +     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +     return adv7511_mode_valid(adv7511, mode);
>> +}
>> +
>> +static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
>> +                                  struct drm_display_mode *mode,
>> +                                  struct drm_display_mode *adj_mode)
>> +{
>> +     struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
>> +
>> +     adv7511_mode_set(adv7511, mode, adj_mode);
>> +}
>> +
>>  static struct drm_encoder_slave_funcs adv7511_encoder_funcs = {
>>       .dpms = adv7511_encoder_dpms,
>>       .mode_valid = adv7511_encoder_mode_valid,
>>       .mode_set = adv7511_encoder_mode_set,
>>       .detect = adv7511_encoder_detect,
>> -     .get_modes = adv7511_get_modes,
>> +     .get_modes = adv7511_encoder_get_modes,
>>  };
>>
>>  /*
>> ---------------------------------------------------------------------------
>> --
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Laurent Pinchart Dec. 3, 2015, 3:28 p.m. UTC | #16
On Thursday 03 December 2015 10:02:02 Rob Clark wrote:
> On Mon, Jul 27, 2015 at 4:59 AM, Laurent Pinchart wrote:
> > On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
> >> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
> >> the other hand, is going be a normal i2c client device creating bridge
> >> and connector entities.
> > 
> > Please, no. It's really time to stop piling hacks and fix the problem
> > properly. There's no reason to have separate APIs for I2C slave encoders
> > and DRM bridges. Those two APIs need to be merged, and then you'll find
> > it much easier to implement ADV7533 support.
> 
> btw, what is the status here?  I'd kind of lost track of the
> discussion, but I'm getting impatient that it is somehow taking
> ridiculously long to get adv7533 support merged.  (It's good thing the
> x86/desktop folks don't bikeshed so much..  I'd hate to wait a year
> for my new laptop to be supported..)

I'd hardly call the overall architecture on which drivers rely a bikeshed (and 
maybe if x86/desktop folks cared more about embedded there would be more 
willingness to make the framework evolve in an embedded-friendly way).

> Anyways, if needed, just copy/paste the adv7511/7533 code into a
> separate bridge-only driver, and we'll use that.  Once the
> i2c-slave-encoder users for adv7511 are converted over, we can delete
> the original slave encoder driver.  That seems like a sane transition
> plan to a bridge-only world.

It's a very sane way to make sure nobody will do the work and keep two copies 
of the same code for a long time, yes.

The path forward is pretty clear, the issue is to find someone who can do the 
work.
Rob Clark Dec. 3, 2015, 3:55 p.m. UTC | #17
On Thu, Dec 3, 2015 at 10:28 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 03 December 2015 10:02:02 Rob Clark wrote:
>> On Mon, Jul 27, 2015 at 4:59 AM, Laurent Pinchart wrote:
>> > On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>> >> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
>> >> the other hand, is going be a normal i2c client device creating bridge
>> >> and connector entities.
>> >
>> > Please, no. It's really time to stop piling hacks and fix the problem
>> > properly. There's no reason to have separate APIs for I2C slave encoders
>> > and DRM bridges. Those two APIs need to be merged, and then you'll find
>> > it much easier to implement ADV7533 support.
>>
>> btw, what is the status here?  I'd kind of lost track of the
>> discussion, but I'm getting impatient that it is somehow taking
>> ridiculously long to get adv7533 support merged.  (It's good thing the
>> x86/desktop folks don't bikeshed so much..  I'd hate to wait a year
>> for my new laptop to be supported..)
>
> I'd hardly call the overall architecture on which drivers rely a bikeshed (and
> maybe if x86/desktop folks cared more about embedded there would be more
> willingness to make the framework evolve in an embedded-friendly way).

I don't know that we can blame this on the desktop folks like that..
after all i2c-slave was sufficient for embedded devices for some time,
and it was an improvement over what came before when it comes to
sharing external encoder support (ie. nothing)

But, as was the case with atomic, the framework evolves.  And as
atomic roll-out has shown, it doesn't require a flag-day if you accept
some duplication.

>> Anyways, if needed, just copy/paste the adv7511/7533 code into a
>> separate bridge-only driver, and we'll use that.  Once the
>> i2c-slave-encoder users for adv7511 are converted over, we can delete
>> the original slave encoder driver.  That seems like a sane transition
>> plan to a bridge-only world.
>
> It's a very sane way to make sure nobody will do the work and keep two copies
> of the same code for a long time, yes.

As opposed to a change everything at once approach, and corresponding
updates to drivers for hw you don't have.  That sounds like a *much*
saner plan</sarcasm>

Seriously, the cost of duplicating a bit of code is low.  Especially
when the code you are duplicating is low churn.  Duplicating lets
other drivers that use adv75xx via slave-encoder migrate over at their
own pace.  Similar to how atomic was rolled out.  To me that sounds
like the much more practical way forward.

> The path forward is pretty clear, the issue is to find someone who can do the
> work.

Maybe the end goal is clear.  But apparently the path forward less so.

BR,
-R

> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Dec. 3, 2015, 4:06 p.m. UTC | #18
Hi Rob,

On Thursday 03 December 2015 10:55:12 Rob Clark wrote:
> On Thu, Dec 3, 2015 at 10:28 AM, Laurent Pinchart wrote:
> > On Thursday 03 December 2015 10:02:02 Rob Clark wrote:
> >> On Mon, Jul 27, 2015 at 4:59 AM, Laurent Pinchart wrote:
> >>> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
> >>>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
> >>>> the other hand, is going be a normal i2c client device creating bridge
> >>>> and connector entities.
> >>> 
> >>> Please, no. It's really time to stop piling hacks and fix the problem
> >>> properly. There's no reason to have separate APIs for I2C slave
> >>> encoders and DRM bridges. Those two APIs need to be merged, and then
> >>> you'll find it much easier to implement ADV7533 support.
> >> 
> >> btw, what is the status here?  I'd kind of lost track of the
> >> discussion, but I'm getting impatient that it is somehow taking
> >> ridiculously long to get adv7533 support merged.  (It's good thing the
> >> x86/desktop folks don't bikeshed so much..  I'd hate to wait a year
> >> for my new laptop to be supported..)
> > 
> > I'd hardly call the overall architecture on which drivers rely a bikeshed
> > (and maybe if x86/desktop folks cared more about embedded there would be
> > more willingness to make the framework evolve in an embedded-friendly
> > way).
>
> I don't know that we can blame this on the desktop folks like that..

To be clear I'm not blaming anyone here, just pointing out that the grass 
isn't all green on the other side ;-)

> after all i2c-slave was sufficient for embedded devices for some time,
> and it was an improvement over what came before when it comes to
> sharing external encoder support (ie. nothing)
>
> But, as was the case with atomic, the framework evolves.  And as
> atomic roll-out has shown, it doesn't require a flag-day if you accept
> some duplication.
> 
> >> Anyways, if needed, just copy/paste the adv7511/7533 code into a
> >> separate bridge-only driver, and we'll use that.  Once the
> >> i2c-slave-encoder users for adv7511 are converted over, we can delete
> >> the original slave encoder driver.  That seems like a sane transition
> >> plan to a bridge-only world.
> > 
> > It's a very sane way to make sure nobody will do the work and keep two
> > copies of the same code for a long time, yes.
> 
> As opposed to a change everything at once approach, and corresponding
> updates to drivers for hw you don't have.  That sounds like a *much*
> saner plan</sarcasm>

There's a single driver using the adv7511 I2C slave encoder, that's the rcar-
du driver. I will make sure to update and test the driver.

> Seriously, the cost of duplicating a bit of code is low.  Especially
> when the code you are duplicating is low churn.  Duplicating lets
> other drivers that use adv75xx via slave-encoder migrate over at their
> own pace.  Similar to how atomic was rolled out.  To me that sounds
> like the much more practical way forward.
> 
> > The path forward is pretty clear, the issue is to find someone who can do
> > the work.
> 
> Maybe the end goal is clear.  But apparently the path forward less so.
Archit Taneja Dec. 3, 2015, 4:11 p.m. UTC | #19
On 12/3/2015 9:25 PM, Rob Clark wrote:
> On Thu, Dec 3, 2015 at 10:28 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> On Thursday 03 December 2015 10:02:02 Rob Clark wrote:
>>> On Mon, Jul 27, 2015 at 4:59 AM, Laurent Pinchart wrote:
>>>> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>>>>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on
>>>>> the other hand, is going be a normal i2c client device creating bridge
>>>>> and connector entities.
>>>>
>>>> Please, no. It's really time to stop piling hacks and fix the problem
>>>> properly. There's no reason to have separate APIs for I2C slave encoders
>>>> and DRM bridges. Those two APIs need to be merged, and then you'll find
>>>> it much easier to implement ADV7533 support.
>>>
>>> btw, what is the status here?  I'd kind of lost track of the
>>> discussion, but I'm getting impatient that it is somehow taking
>>> ridiculously long to get adv7533 support merged.  (It's good thing the
>>> x86/desktop folks don't bikeshed so much..  I'd hate to wait a year
>>> for my new laptop to be supported..)
>>
>> I'd hardly call the overall architecture on which drivers rely a bikeshed (and
>> maybe if x86/desktop folks cared more about embedded there would be more
>> willingness to make the framework evolve in an embedded-friendly way).
>
> I don't know that we can blame this on the desktop folks like that..
> after all i2c-slave was sufficient for embedded devices for some time,
> and it was an improvement over what came before when it comes to
> sharing external encoder support (ie. nothing)
>
> But, as was the case with atomic, the framework evolves.  And as
> atomic roll-out has shown, it doesn't require a flag-day if you accept
> some duplication.
>
>>> Anyways, if needed, just copy/paste the adv7511/7533 code into a
>>> separate bridge-only driver, and we'll use that.  Once the
>>> i2c-slave-encoder users for adv7511 are converted over, we can delete
>>> the original slave encoder driver.  That seems like a sane transition
>>> plan to a bridge-only world.
>>
>> It's a very sane way to make sure nobody will do the work and keep two copies
>> of the same code for a long time, yes.
>
> As opposed to a change everything at once approach, and corresponding
> updates to drivers for hw you don't have.  That sounds like a *much*
> saner plan</sarcasm>
>
> Seriously, the cost of duplicating a bit of code is low.  Especially
> when the code you are duplicating is low churn.  Duplicating lets
> other drivers that use adv75xx via slave-encoder migrate over at their
> own pace.  Similar to how atomic was rolled out.  To me that sounds
> like the much more practical way forward.
>
>> The path forward is pretty clear, the issue is to find someone who can do the
>> work.

I volunteer (as tribute)!

Once we settle on the best way to do it.

>
> Maybe the end goal is clear.  But apparently the path forward less so.

Laurent, as we'd discussed in the past, I did an initial study of how
we could map bridge ops to the encoder slave ops, and change it for the
rcar kms driver (and others) that might use adv7511.

The drm_encoder_slave_funcs ops are a superset of bridge ops. The 
rcar-du driver uses a subset of these ops (encoder-like ops) to
implement the hdmi encoder (rcar_du_hdmienc.c) and the other 
(connector-like ops)for implementing the hdmi connector
(rcar_du_hdmicon.c)

We have two options:

1. If we want to use drm_bridge as it is, the rcar driver shouldn't
create a hdmi connector anymore, and rely on the adv7511 driver to make
a connector for it. This is the easiest way, but it will break the nice
representation of the hardware in DT.

2. We add more drm_bridge ops (the ones that implement the connector 
ops), and make the hdmi connector created by rcar-du use these bridge
ops.

Both these options have issues. The 2) one is the probably the better of
the two, but I don't know if adding more ops to the bridge is the right
way to go.

Therefore, I don't know how we can solve this straight away. If there is
more debate needed on this, then it is probably easier to get adv7533
support in, and then figure out how two merge the two.

Archit

>
> BR,
> -R
>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
Archit Taneja Jan. 9, 2016, 5:03 p.m. UTC | #20
Hi Laurent,

On 12/3/2015 9:41 PM, Archit Taneja wrote:
>
>
> On 12/3/2015 9:25 PM, Rob Clark wrote:
>> On Thu, Dec 3, 2015 at 10:28 AM, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>>> On Thursday 03 December 2015 10:02:02 Rob Clark wrote:
>>>> On Mon, Jul 27, 2015 at 4:59 AM, Laurent Pinchart wrote:
>>>>> On Monday 27 July 2015 11:46:57 Archit Taneja wrote:
>>>>>> ADV7511 is represented as an i2c drm slave encoder device.
>>>>>> ADV7533, on
>>>>>> the other hand, is going be a normal i2c client device creating
>>>>>> bridge
>>>>>> and connector entities.
<snip>

>
> Laurent, as we'd discussed in the past, I did an initial study of how
> we could map bridge ops to the encoder slave ops, and change it for the
> rcar kms driver (and others) that might use adv7511.
>
> The drm_encoder_slave_funcs ops are a superset of bridge ops. The
> rcar-du driver uses a subset of these ops (encoder-like ops) to
> implement the hdmi encoder (rcar_du_hdmienc.c) and the other
> (connector-like ops)for implementing the hdmi connector
> (rcar_du_hdmicon.c)
>
> We have two options:
>
> 1. If we want to use drm_bridge as it is, the rcar driver shouldn't
> create a hdmi connector anymore, and rely on the adv7511 driver to make
> a connector for it. This is the easiest way, but it will break the nice
> representation of the hardware in DT.
>
> 2. We add more drm_bridge ops (the ones that implement the connector
> ops), and make the hdmi connector created by rcar-du use these bridge
> ops.
>
> Both these options have issues. The 2) one is the probably the better of
> the two, but I don't know if adding more ops to the bridge is the right
> way to go.
>
> Therefore, I don't know how we can solve this straight away. If there is
> more debate needed on this, then it is probably easier to get adv7533
> support in, and then figure out how two merge the two.
>

I gave approach (1) a try. I modified the rcar-du driver to use bridge
instead of i2c slave encoder and posted a RFC [1]. The hdmi connector
DT nodes (like in r8a7790-lager.dts) aren't really used by the rcar-du
driver. Now, with the adv7511 driver creating its own connector, they
mean even much less.

The adv7511 refactor from i2c slave encoder to bridge is done in [2].

[1] http://marc.info/?l=dri-devel&m=145235835902378&w=1
[2] http://marc.info/?l=dri-devel&m=145235823602353&w=1

Could you please review and test?

Thanks,
Archit

>
>>
>> BR,
>> -R
>>
>>> --
>>> Regards,
>>>
>>> Laurent Pinchart
>>>
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
index 63a3d20..46fb24d 100644
--- a/drivers/gpu/drm/i2c/adv7511.c
+++ b/drivers/gpu/drm/i2c/adv7511.c
@@ -612,13 +612,11 @@  static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 }
 
 /* -----------------------------------------------------------------------------
- * Encoder operations
+ * ADV75xx helpers
  */
-
-static int adv7511_get_modes(struct drm_encoder *encoder,
-			     struct drm_connector *connector)
+static int adv7511_get_modes(struct adv7511 *adv7511,
+		struct drm_connector *connector)
 {
-	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
 	struct edid *edid;
 	unsigned int count;
 
@@ -656,21 +654,10 @@  static int adv7511_get_modes(struct drm_encoder *encoder,
 	return count;
 }
 
-static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
-{
-	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
-
-	if (mode == DRM_MODE_DPMS_ON)
-		adv7511_power_on(adv7511);
-	else
-		adv7511_power_off(adv7511);
-}
-
 static enum drm_connector_status
-adv7511_encoder_detect(struct drm_encoder *encoder,
+adv7511_detect(struct adv7511 *adv7511,
 		       struct drm_connector *connector)
 {
-	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
 	enum drm_connector_status status;
 	unsigned int val;
 	bool hpd;
@@ -694,7 +681,7 @@  adv7511_encoder_detect(struct drm_encoder *encoder,
 	if (status == connector_status_connected && hpd && adv7511->powered) {
 		regcache_mark_dirty(adv7511->regmap);
 		adv7511_power_on(adv7511);
-		adv7511_get_modes(encoder, connector);
+		adv7511_get_modes(adv7511, connector);
 		if (adv7511->status == connector_status_connected)
 			status = connector_status_disconnected;
 	} else {
@@ -708,8 +695,8 @@  adv7511_encoder_detect(struct drm_encoder *encoder,
 	return status;
 }
 
-static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
-				      struct drm_display_mode *mode)
+static int adv7511_mode_valid(struct adv7511 *adv7511,
+				     const struct drm_display_mode *mode)
 {
 	if (mode->clock > 165000)
 		return MODE_CLOCK_HIGH;
@@ -717,11 +704,10 @@  static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
 	return MODE_OK;
 }
 
-static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
+static void adv7511_mode_set(struct adv7511 *adv7511,
 				     struct drm_display_mode *mode,
 				     struct drm_display_mode *adj_mode)
 {
-	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
 	unsigned int low_refresh_rate;
 	unsigned int hsync_polarity = 0;
 	unsigned int vsync_polarity = 0;
@@ -812,12 +798,60 @@  static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
 	adv7511->f_tmds = mode->clock;
 }
 
+/* -----------------------------------------------------------------------------
+ * Encoder operations
+ */
+
+static int adv7511_encoder_get_modes(struct drm_encoder *encoder,
+			     struct drm_connector *connector)
+{
+	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
+
+	return adv7511_get_modes(adv7511, connector);
+}
+
+static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
+{
+	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
+
+	if (mode == DRM_MODE_DPMS_ON)
+		adv7511_power_on(adv7511);
+	else
+		adv7511_power_off(adv7511);
+}
+
+static enum drm_connector_status
+adv7511_encoder_detect(struct drm_encoder *encoder,
+		       struct drm_connector *connector)
+{
+	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
+
+	return adv7511_detect(adv7511, connector);
+}
+
+static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
+				      struct drm_display_mode *mode)
+{
+	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
+
+	return adv7511_mode_valid(adv7511, mode);
+}
+
+static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
+				     struct drm_display_mode *mode,
+				     struct drm_display_mode *adj_mode)
+{
+	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
+
+	adv7511_mode_set(adv7511, mode, adj_mode);
+}
+
 static struct drm_encoder_slave_funcs adv7511_encoder_funcs = {
 	.dpms = adv7511_encoder_dpms,
 	.mode_valid = adv7511_encoder_mode_valid,
 	.mode_set = adv7511_encoder_mode_set,
 	.detect = adv7511_encoder_detect,
-	.get_modes = adv7511_get_modes,
+	.get_modes = adv7511_encoder_get_modes,
 };
 
 /* -----------------------------------------------------------------------------