[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
Archit Taneja
architt at codeaurora.org
Thu Oct 20 11:26:44 UTC 2016
On 10/20/2016 02:45 PM, Russell King - ARM Linux wrote:
> On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote:
>>
>>
>> On 10/20/2016 01:50 PM, Laurent Pinchart wrote:
>>> Hi Russell,
>>>
>>> On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
>>>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
>>>>> On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
>>>>>> In any case, I don't agree with converting it to a DRM bridge - that
>>>>>> will mean that we have to split the driver into two pieces, the bridge
>>>>>> part handling the mode set specifics, and a connector/encoder part which
>>>>>> handles the detection/edid stuff.
>>>>>>
>>>>>> We might as well keep the whole thing as the classical connector/encoder
>>>>>> rather than introducing this additional layer of complexity.
>>>>>
>>>>> We have different ways to instantiate external HDMI encoders, and that's
>>>>> not good. I believe everybody agrees that drm encoder slave has to go, so
>>>>> that's already one problem solved (or rather solvable, it still requires
>>>>> someone to do the work). We will then be left with two different methods,
>>>>> drm bridge and non-bridge component-based instantiation. We need to
>>>>> somehow merge the two, and I'm open to discussions on how the end result
>>>>> should look like.
>>>>
>>>> I think you're idea really doesn't work - and I think your idea that
>>>> component-based is somehow separate from other methods is wrong.
>>>>
>>>> Look at iMX for example - even converting all the code that could be
>>>> a bridge to be a bridge will not get rid of its use of the component
>>>> instantiation, because you still have other components that need to
>>>> come from elsewhere. The same is true of armada as well.
>>>
>>> Don't get me wrong, I'm certainly not against the component framework itself.
>>> A way to bind multiple independent devices together is needed. We have a
>>> similar framework in V4L2 called v4l2-async, and I'd like to see it moved to
>>> the component framework at some point to unify code. Some changes to the
>>> component framework might be needed to support needs of V4L2 that are
>>> currently not applicable to DRM/KMS, but there's nothing major there.
>>>
>>>> Moreover, as I've already said, converting tda998x to a DRM bridge
>>>> will not get rid of the encoder/connector part, because it _is_ a
>>>> connector in the DRM sense - it provides the detection and EDID
>>>> reading.
>>>>
>>>> So, what would we achieve by splitting the driver into a DRM bridge
>>>> and DRM encoder/connector?
>>>
>>> Please note that DRM bridge doesn't split the DRM connector out of the bridge,
>>> bridges instantiate drm_connector objects. In that sense they don't differ
>>> much from the model implemented by the tda998x driver.
>>>
>>> I however believe that connectors should be split out DRM bridge drivers, for
>>> the simple reason that bridges can be chained. The output of a bridge isn't
>>> guaranteed to be a connector but can be another bridge. We managed not to have
>>> to deal with that in a generic way so far in mainline, but we'll run into the
>>> problem one of these days, and a solution will be needed. There's no rush
>>> right now, and this is unrelated to converting tda998x to DRM bridge.
>>>
>>>> We would still need the component helper to manage the binding of
>>>> the connector part, which would also then have to register a DRM
>>>> bridge in addition to a DRM encoder and providing the DRM connector
>>>> as we can't have two device drivers bound to the same i2c device.
>>>> What we get is more complexity in the driver.
>>>
>>> DRM bridges indeed don't create encoders. That task is left to the display
>>> driver. The reason is the same as above: bridges can be chained (including
>>> with an internal encoder that is not modelled as a bridge, and that's a case
>>> we have today), while the KMS model exposes a single encoder to userspace.
>>> Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
>>> Better solutions would have been to expose no encoder at all or all encoders
>>> in the chain. We are however stuck with this model as we can't break the UABI.
>>> For that reason a DRM encoder object doesn't represent an encoder but a chain
>>> of encoders. As a DRM bridge models a single encoder, the DRM encoder object
>>> must be created at a higher level, in the display driver.
>>
>> One more thing is that the TDA998x in its current form won't work
>> with KMS drivers that create their own drm_encoder objects to represent
>> their hardware's mipi DPI/RGB interfaces. For such drivers, we would
>> want the TDA998x to tie itself to the existing encoder created by the
>> KMS driver, rather than creating its own.
>
> Please show _technically_ how this would work. I want to see code or
> pseudo-code illustrating how a "foreign" DRM encoder could be used with
> either dw-hdmi or tda998x, because right now I can't see any way that
> could work.
This is something we already do with the adv7511 bridge driver on msm,
rcar and arc (for 4.9) drivers.
I've shared pseudo code on the kms driver and encoder chip's driver
side. I've also shared a diff that converts the tda998x driver to use
drm_bridge(uncompiled/untested).
1) Kms driver side:
/*
* Create an encoder instance. Depending on the hardware represented
* by the KMS driver, the encoder can ops can either have some
* functionality, or be nops. In the case of tilcdc, the encoder
* funcs would be mostly nops.
*/
drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs);
drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs,
type, NULL);
/*
* Extract the bridge using DT node of the external encoder chip,
* i.e. tda998x
*/
bridge = of_drm_find_bridge(encoder_chip_dev->of_node);
/*
* Tell our newly created drm_encoder that it is connected
* to a bridge. The atomic helpers and legacy crtc helpers
* check if the encoder is connected to a bridge. If so,
* it'll call the bridge ops along with the encoder ops.
*/
bridge->encoder = &kms_priv->encoder;
kms_priv->encoder.bridge = bridge;
drm_bridge_attach(kms_priv->drm, bridge);
...
...
2) On the encoder chip driver side:
/*
* The drm bridge driver creates only drm_bridge and connector,
* it assumes that the drm_encoder the bridge is tied to is
* created by the kms driver. When the KMS driver calls
* drm_bridge_attach with the bridge pointer, it assumes that
* we have already set up the links between the encoder
* and the bridge.
*/
static int tda998x_bridge_attach(struct drm_bridge *bridge)
{
/* set up connector here, you can peek at the
* drm_encoder by referencing bridge->encoder
*/
...
}
static struct drm_bridge_funcs tda998x_bridge_funcs = {
/* ops that are very similar to encoder ops */
.enable = tda998x_bridge_enable,
.disable = tda998x_bridge_disable,
.mode_set = tda998x_bridge_mode_set,
/* the attach op that binds the bridge to the kms device */
.attach = tda998x_bridge_attach,
};
static int tda998x_bind(...)
{
...
...
priv->bridge.funcs = &tda998x_bridge_funcs;
priv->bridge.of_node = dev->of_node;
ret = drm_bridge_add(&priv->bridge);
...
...
}
3) Rough conversion to bridge:
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9798d40..bdca061 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -60,7 +60,7 @@ struct tda998x_priv {
wait_queue_head_t edid_delay_waitq;
bool edid_delay_active;
- struct drm_encoder encoder;
+ struct drm_bridge bridge;
struct drm_connector connector;
struct tda998x_audio_port audio_port[2];
@@ -69,8 +69,8 @@ struct tda998x_priv {
#define conn_to_tda998x_priv(x) \
container_of(x, struct tda998x_priv, connector)
-#define enc_to_tda998x_priv(x) \
- container_of(x, struct tda998x_priv, encoder)
+#define bridge_to_tda998x_priv(x) \
+ container_of(x, struct tda998x_priv, bridge)
/* The TDA9988 series of devices use a paged register scheme.. to simplify
* things we encode the page # in upper bits of the register #. To read/
@@ -614,7 +614,7 @@ static void tda998x_detect_work(struct work_struct
*work)
{
struct tda998x_priv *priv =
container_of(work, struct tda998x_priv, detect_work);
- struct drm_device *dev = priv->encoder.dev;
+ struct drm_device *dev = priv->connector.dev;
if (dev)
drm_kms_helper_hotplug_event(dev);
@@ -840,9 +840,9 @@ static void tda998x_encoder_set_config(struct
tda998x_priv *priv,
priv->audio_params = p->audio_params;
}
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+static void tda998x_encoder_dpms(struct drm_bridge *bridge, int mode)
{
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
/* we only care about on or off: */
if (mode != DRM_MODE_DPMS_ON)
@@ -889,11 +889,11 @@ static int tda998x_connector_mode_valid(struct
drm_connector *connector,
}
static void
-tda998x_encoder_mode_set(struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+tda998x_bridge_mode_set(struct drm_bridge *bridge,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
{
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
u16 ref_pix, ref_line, n_pix, n_line;
u16 hs_pix_s, hs_pix_e;
u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1227,7 +1227,8 @@ static int tda998x_audio_hw_params(struct device
*dev, void *data,
.cea = params->cea,
};
- if (!priv->encoder.crtc)
+ /* ?? */
+ if (!priv->bridge.encoder->crtc)
return -ENODEV;
memcpy(audio.status, params->iec.status,
@@ -1267,7 +1268,7 @@ static int tda998x_audio_hw_params(struct device
*dev, void *data,
mutex_lock(&priv->audio_mutex);
ret = tda998x_configure_audio(priv,
&audio,
- priv->encoder.crtc->hwmode.clock);
+ priv->bridge.encoder->crtc->hwmode.clock);
if (ret == 0)
priv->audio_params = audio;
@@ -1538,41 +1539,12 @@ static int tda998x_create(struct i2c_client
*client, struct tda998x_priv *priv)
return -ENXIO;
}
-static void tda998x_encoder_prepare(struct drm_encoder *encoder)
-{
- tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
-}
-
-static void tda998x_encoder_commit(struct drm_encoder *encoder)
-{
- tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
-}
-
-static const struct drm_encoder_helper_funcs
tda998x_encoder_helper_funcs = {
- .dpms = tda998x_encoder_dpms,
- .prepare = tda998x_encoder_prepare,
- .commit = tda998x_encoder_commit,
- .mode_set = tda998x_encoder_mode_set,
-};
-
-static void tda998x_encoder_destroy(struct drm_encoder *encoder)
-{
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
-
- tda998x_destroy(priv);
- drm_encoder_cleanup(encoder);
-}
-
-static const struct drm_encoder_funcs tda998x_encoder_funcs = {
- .destroy = tda998x_encoder_destroy,
-};
-
static struct drm_encoder *
tda998x_connector_best_encoder(struct drm_connector *connector)
{
struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
- return &priv->encoder;
+ return priv->bridge.encoder;
}
static
@@ -1584,7 +1556,6 @@ static void tda998x_encoder_destroy(struct
drm_encoder *encoder)
static void tda998x_connector_destroy(struct drm_connector *connector)
{
- drm_connector_unregister(connector);
drm_connector_cleanup(connector);
}
@@ -1606,13 +1577,50 @@ static int tda998x_connector_dpms(struct
drm_connector *connector, int mode)
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};
+static void tda998x_bridge_enable(struct drm_bridge *bridge)
+{
+ tda998x_encoder_dpms(bridge, DRM_MODE_DPMS_ON);
+}
+
+static void tda998x_bridge_disable(struct drm_bridge *bridge)
+{
+ tda998x_encoder_dpms(bridge, DRM_MODE_DPMS_OFF);
+}
+
+static int tda998x_bridge_attach(struct drm_bridge *bridge)
+{
+ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+ struct drm_connector *connector = &priv->connector;
+ struct drm_device *dev = dev_get_drvdata(
+
+ connector->interlace_allowed = 1;
+ tda998x_encoder_set_polling(priv, connector);
+
+ drm_connector_helper_add(connector, &tda998x_connector_helper_funcs);
+ ret = drm_connector_init(drm, connector,
+ &tda998x_connector_funcs,
+ DRM_MODE_CONNECTOR_HDMIA);
+ if (ret)
+ return ret;
+
+ drm_mode_connector_attach_encoder(&priv->connector, priv->bridge.encoder);
+
+ return 0;
+}
+
+static struct drm_bridge_funcs tda998x_bridge_funcs = {
+ .enable = tda998x_bridge_enable,
+ .disable = tda998x_bridge_disable,
+ .mode_set = tda998x_bridge_mode_set,
+ .attach = tda998x_bridge_attach,
+};
+
static int tda998x_bind(struct device *dev, struct device *master,
void *data)
{
struct tda998x_encoder_params *params = dev->platform_data;
struct i2c_client *client = to_i2c_client(dev);
struct drm_device *drm = data;
struct tda998x_priv *priv;
- u32 crtcs = 0;
int ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -1621,17 +1629,7 @@ static int tda998x_bind(struct device *dev,
struct device *master, void *data)
dev_set_drvdata(dev, priv);
- if (dev->of_node)
- crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
-
- /* If no CRTCs were found, fall back to our old behaviour */
- if (crtcs == 0) {
- dev_warn(dev, "Falling back to first CRTC\n");
- crtcs = 1 << 0;
- }
-
- priv->connector.interlace_allowed = 1;
- priv->encoder.possible_crtcs = crtcs;
+ /* find possible crtcs in the KMS driver */
ret = tda998x_create(client, priv);
if (ret)
@@ -1640,35 +1638,18 @@ static int tda998x_bind(struct device *dev,
struct device *master, void *data)
if (!dev->of_node && params)
tda998x_encoder_set_config(priv, params);
- tda998x_encoder_set_polling(priv, &priv->connector);
-
- drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
- ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
- DRM_MODE_ENCODER_TMDS, NULL);
- if (ret)
- goto err_encoder;
-
- drm_connector_helper_add(&priv->connector,
- &tda998x_connector_helper_funcs);
- ret = drm_connector_init(drm, &priv->connector,
- &tda998x_connector_funcs,
- DRM_MODE_CONNECTOR_HDMIA);
- if (ret)
- goto err_connector;
-
- ret = drm_connector_register(&priv->connector);
- if (ret)
- goto err_sysfs;
+ priv->bridge.funcs = &tda998x_bridge_funcs;
+ priv->bridge.of_node = dev->of_node;
- drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
+ ret = drm_bridge_add(&priv->bridge);
+ if (ret) {
+ dev_err(dev, "failed to add adv7511 bridge\n");
+ goto err_bridge_add;
+ }
return 0;
-err_sysfs:
- drm_connector_cleanup(&priv->connector);
-err_connector:
- drm_encoder_cleanup(&priv->encoder);
-err_encoder:
+err_bridge_add:
tda998x_destroy(priv);
return ret;
}
@@ -1678,9 +1659,8 @@ static void tda998x_unbind(struct device *dev,
struct device *master,
{
struct tda998x_priv *priv = dev_get_drvdata(dev);
- drm_connector_unregister(&priv->connector);
drm_connector_cleanup(&priv->connector);
- drm_encoder_cleanup(&priv->encoder);
+ drm_bridge_remove(&priv->bridge);
tda998x_destroy(priv);
}
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
More information about the dri-devel
mailing list