[PATCH] drm: i2c: adv7511: Convert to drm_bridge
Archit Taneja
architt at codeaurora.org
Sun Jan 10 21:49:07 PST 2016
On 01/11/2016 07:38 AM, Laurent Pinchart wrote:
> Hi Archit,
>
> Thanks a lot for the patch.
>
> On Saturday 09 January 2016 22:20:25 Archit Taneja wrote:
>> We don't want to use the old i2c slave encoder interface anymore.
>
> I happily agree with that :-)
>
>> Remove that and make the i2c driver create a drm_bridge entity instead.
>> Converting to bridges helps because the kms drivers don't need to
>> exract encoder slave ops from this driver and use it within their
>> own encoder/connector ops.
>>
>> The driver now creates its own connector when a kms driver attaches
>> itself to the bridge. Therefore, kms drivers don't need to create
>> their own connectors anymore.
>
> That, however, I don't agree with. There's nothing that guarantees that the
> output of a bridge is a connector. The bridge output could be connected to the
> input of another bridge. Quoting drm_bridge.c,
>
> * A bridge is always attached to a single &drm_encoder at a time, but can be
> * either connected to it directly, or through an intermediate bridge:
> *
> * encoder ---> bridge B ---> bridge A
>
> Creating the connector should be done by the DRM driver, as that's the only
> driver that should have knowledge of the full hardware topology.
I mostly agree with your point. I think this was discussed in the
previous adv7511 thread too, but we didn't come up with a way to do it
without using encoder chains that Boris had proposed.
In the case of adv7511, I don't think there'll be a board that would
ever use adv7511 as an intermediate encoder chip. If they do, it would
be quite pointless. In fact, if you see any bridge driver in the kernel
at the moment, they all create their own connectors.
About the comment you quoted from drm_bridge.c, this method of daisy
chaining bridges was primarily added because some kms drivers tend to
use up a bridge to represent a piece of the encoder HW within the SoC
itself. In other words,"bridge B" in the diagram above hasn't been
really used for an external encoder chip until now.
I do agree that it would be better if the kms driver could create the
connector, and somehow retrieve ops from the adv7511 driver, but I
can't think of a good way to do that.
At the moment, I don't think it is a big issue. We might want to revisit
it when we really have encoder chips that can sit somewhere in the
middle of an encoder chain.
>
> Apart from that the patch looks good to me, please see below for a couple of
> comments.
>
> As creating the connector outside of the bridge driver will significantly
> impact "[RFC] drm: rcar-du: Remove i2c slave encoder interface for hdmi
> encoder" I'll postpone reviewing that patch until we reach an agreement on
> this one if that's fine with you. Please let me know if there are items in the
> rcar-du patch that you would like me to already look at.
Sure. One thing I noticed in the rcar-du driver when it creates the
hdmi output chain is that it looks for the hdmi connector node in DT,
and bails out if it doesn't find one, but it doesn't do anything with it
apart from that. Any reason why we created a hdmi connector device via
DT and not use it?
>
>> The old encoder slave ops are now used by the new bridge and connector
>> entities.
>>
>> Signed-off-by: Archit Taneja <architt at codeaurora.org>
>> ---
>> drivers/gpu/drm/i2c/adv7511.c | 234 ++++++++++++++++++++++++++-------------
>> 1 file changed, 160 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
>> index 00416f2..3fda10f 100644
>> --- a/drivers/gpu/drm/i2c/adv7511.c
>> +++ b/drivers/gpu/drm/i2c/adv7511.c
>> @@ -16,7 +16,8 @@
>> #include <drm/drmP.h>
>> #include <drm/drm_crtc_helper.h>
>> #include <drm/drm_edid.h>
>> -#include <drm/drm_encoder_slave.h>
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_atomic_helper.h>
>
> Alphabetical order please :-)
Sure, I'll fix this.
>
>>
>> #include "adv7511.h"
>>
>> @@ -36,7 +37,8 @@ struct adv7511 {
>> bool edid_read;
>>
>> wait_queue_head_t wq;
>> - struct drm_encoder *encoder;
>> + struct drm_bridge bridge;
>> + struct drm_connector connector;
>>
>> bool embedded_sync;
>> enum adv7511_sync_polarity vsync_polarity;
>> @@ -48,11 +50,6 @@ struct adv7511 {
>> struct gpio_desc *gpio_pd;
>> };
>>
>> -static struct adv7511 *encoder_to_adv7511(struct drm_encoder *encoder)
>> -{
>> - return to_encoder_slave(encoder)->slave_priv;
>> -}
>> -
>> /* ADI recommended values for proper operation. */
>> static const struct reg_sequence adv7511_fixed_registers[] = {
>> { 0x98, 0x03 },
>> @@ -438,8 +435,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511)
>> regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
>> regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>>
>> - if (irq0 & ADV7511_INT0_HDP && adv7511->encoder)
>> - drm_helper_hpd_irq_event(adv7511->encoder->dev);
>> + if (irq0 & ADV7511_INT0_HDP && adv7511->bridge.encoder)
>> + drm_helper_hpd_irq_event(adv7511->connector.dev);
>>
>> if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
>> adv7511->edid_read = true;
>> @@ -555,13 +552,12 @@ 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,
>> +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;
>>
>> @@ -596,21 +592,9 @@ 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,
>> - struct drm_connector *connector)
>> +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;
>> @@ -634,7 +618,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 {
>> @@ -648,8 +632,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,
>> + struct drm_display_mode *mode)
>> {
>> if (mode->clock > 165000)
>> return MODE_CLOCK_HIGH;
>> @@ -657,11 +641,10 @@ static int adv7511_encoder_mode_valid(struct
>> drm_encoder *encoder, return MODE_OK;
>> }
>>
>> -static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
>> - struct drm_display_mode *mode,
>> - struct drm_display_mode *adj_mode)
>> +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;
>> @@ -752,12 +735,132 @@ static void adv7511_encoder_mode_set(struct
>> drm_encoder *encoder, adv7511->f_tmds = mode->clock;
>> }
>>
>> -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,
>> +/* Connector funcs */
>> +static struct adv7511 *connector_to_adv7511(struct drm_connector
>> *connector) +{
>> + return container_of(connector, struct adv7511, connector);
>> +}
>> +
>> +static int adv7511_connector_get_modes(struct drm_connector *connector)
>> +{
>> + struct adv7511 *adv = connector_to_adv7511(connector);
>> +
>> + return adv7511_get_modes(adv, connector);
>> +}
>> +
>> +static struct drm_encoder *
>> +adv7511_connector_best_encoder(struct drm_connector *connector)
>> +{
>> + struct adv7511 *adv = connector_to_adv7511(connector);
>> +
>> + return adv->bridge.encoder;
>> +}
>> +
>> +static enum drm_mode_status
>> +adv7511_connector_mode_valid(struct drm_connector *connector,
>> + struct drm_display_mode *mode)
>> +{
>> + struct adv7511 *adv = connector_to_adv7511(connector);
>> +
>> + return adv7511_mode_valid(adv, mode);
>> +}
>> +
>> +static struct drm_connector_helper_funcs adv7511_connector_helper_funcs = {
>> + .get_modes = adv7511_connector_get_modes,
>> + .best_encoder = adv7511_connector_best_encoder,
>> + .mode_valid = adv7511_connector_mode_valid,
>> +};
>> +
>> +static enum drm_connector_status
>> +adv7511_connector_detect(struct drm_connector *connector, bool force)
>> +{
>> + struct adv7511 *adv = connector_to_adv7511(connector);
>> +
>> + return adv7511_detect(adv, connector);
>> +}
>> +
>> +static struct drm_connector_funcs adv7511_connector_funcs = {
>> + .dpms = drm_atomic_helper_connector_dpms,
>> + .fill_modes = drm_helper_probe_single_connector_modes,
>> + .detect = adv7511_connector_detect,
>> + .destroy = drm_connector_cleanup,
>> + .reset = drm_atomic_helper_connector_reset,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +/* Bridge funcs */
>> +static struct adv7511 *bridge_to_adv7511(struct drm_bridge *bridge)
>> +{
>> + return container_of(bridge, struct adv7511, bridge);
>> +}
>> +
>> +static void adv7511_bridge_pre_enable(struct drm_bridge *bridge)
>> +{
>> + struct adv7511 *adv = bridge_to_adv7511(bridge);
>> +
>> + adv7511_power_on(adv);
>> +}
>> +
>> +static void adv7511_bridge_post_disable(struct drm_bridge *bridge)
>> +{
>> + struct adv7511 *adv = bridge_to_adv7511(bridge);
>> +
>> + adv7511_power_off(adv);
>> +}
>> +
>> +static void adv7511_bridge_enable(struct drm_bridge *bridge)
>> +{
>> +}
>> +
>> +static void adv7511_bridge_disable(struct drm_bridge *bridge)
>> +{
>> +}
>
> You could define a single function called adv7511_bridge_noop(), or, better,
> make the callbacks optionals in drm_bridge.c.
Sure, I'll go with the noop way.
>
> By the way, what's the reason you use the pre_enable/post_disable callbacks
> instead of enable/disable ?
I don't recollect it at the moment. I'd used these while using adv7533
connected to the dsi bridge in the msm kms driver. I will look into this
and get back.
Thanks for the review,
Archit
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, hosted by The Linux Foundation
More information about the dri-devel
mailing list