drm/arm/komeda: Remove component framework and add a simple encoder

  The Komeda driver always expects the remote connector node to initialize
an encoder. It uses the component aggregator framework which consists
of component->bind() calls used to initialize the remote encoder and attach
it to the crtc. This is different from other drm implementations which expect
the display driver to supply a crtc and connect an encoder to it.

Remove all component framework calls from the komeda driver and declare and
attach an encoder inside komeda_crtc_add().

The remote connector driver has to implement the DRM bridge APIs which
can be used to glue the encoder to the remote connector.

Signed-off-by: Faiz Abbas <faiz.abbas@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  | 22 +++++++-
 .../gpu/drm/arm/display/komeda/komeda_drv.c   | 56 ++-----------------
 .../gpu/drm/arm/display/komeda/komeda_kms.c   |  4 --
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 +
 4 files changed, 30 insertions(+), 55 deletions(-)
  

Hi Faiz,

Thanks for the patch and for addressing what was at some moment on my "nice to
improve / cleanup" list. Sorry for the delay in responding, I had to revive
the bits of an old setup to be able to test this properly, with 2 encoders
attached.

On Wed, Jun 21, 2023 at 02:11:16PM +0530, Faiz Abbas wrote:
> The Komeda driver always expects the remote connector node to initialize
> an encoder. It uses the component aggregator framework which consists
> of component->bind() calls used to initialize the remote encoder and attach
> it to the crtc. This is different from other drm implementations which expect
> the display driver to supply a crtc and connect an encoder to it.

I think both approaches are valid in DRM. We don't want to remove the component
framework because it is doing the wrong thing, but because we cannot use it
with drivers that implement the drm_bridge API. Given that we usually pair with
a component encoder that also implements a drm_bridge, dropping support for
component framework should not affect the users of the driver.

> 
> Remove all component framework calls from the komeda driver and declare and
> attach an encoder inside komeda_crtc_add().

Unfortunately you haven't removed all component framework calls. The hint for
that is that you have not removed the #include <linux/component.h> line from
any of the files. Specifically, komeda_kms_attach()/detach() still calls
component_unbind_all() which will crash with your patch applied.

> 
> The remote connector driver has to implement the DRM bridge APIs which
> can be used to glue the encoder to the remote connector.
> 
> Signed-off-by: Faiz Abbas <faiz.abbas@arm.com>
> ---
>  .../gpu/drm/arm/display/komeda/komeda_crtc.c  | 22 +++++++-
>  .../gpu/drm/arm/display/komeda/komeda_drv.c   | 56 ++-----------------
>  .../gpu/drm/arm/display/komeda/komeda_kms.c   |  4 --
>  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 +
>  4 files changed, 30 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index cea3fd5772b57..144736a69b0ee 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -12,6 +12,8 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
> +#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_bridge.h>
>  
>  #include "komeda_dev.h"
>  #include "komeda_kms.h"
> @@ -612,9 +614,11 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>  			   struct komeda_crtc *kcrtc)
>  {
>  	struct drm_crtc *crtc = &kcrtc->base;
> +	struct drm_device *base = &kms->base;
> +	struct drm_bridge *bridge;
>  	int err;
>  
> -	err = drm_crtc_init_with_planes(&kms->base, crtc,
> +	err = drm_crtc_init_with_planes(base, crtc,
>  					get_crtc_primary(kms, kcrtc), NULL,
>  					&komeda_crtc_funcs, NULL);
>  	if (err)
> @@ -626,6 +630,22 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>  
>  	drm_crtc_enable_color_mgmt(crtc, 0, true, KOMEDA_COLOR_LUT_SIZE);

I would move this line after the bridges are being initialised, just in case in the future
colour management will propagate some info down to the bridges.

>  
> +	/* Construct an encoder for each pipeline and attach it to the remote
> +	 * bridge
> +	 */
> +	kcrtc->encoder.possible_crtcs = drm_crtc_mask(crtc);
> +	err = drm_simple_encoder_init(base, &kcrtc->encoder,
> +				      DRM_MODE_ENCODER_TMDS);
> +	if (err)
> +		return err;
> +
> +	bridge = devm_drm_of_get_bridge(base->dev, kcrtc->master->of_node,
> +					KOMEDA_OF_PORT_OUTPUT, 0);
> +	if (IS_ERR(bridge))
> +		return PTR_ERR(bridge);

I'm a bit undecided on whether to make the absence of a bridge here fatal or not.
For the second pipeline it should be possible to act as a slave to the first
pipeline even if it doesn't have a bridge attached, but I need to investigate a
bit more here. The bindings suggest that this is mandatory, so keep it for now.


> +
> +	err = drm_bridge_attach(&kcrtc->encoder, bridge, NULL, 0);
> +
>  	return err;
>  }
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 28f76e07dd958..70b1b693c6eff 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -28,13 +28,11 @@ struct komeda_dev *dev_to_mdev(struct device *dev)
>  	return mdrv ? mdrv->mdev : NULL;
>  }
>  
> -static void komeda_unbind(struct device *dev)
> +static int komeda_platform_remove(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct komeda_drv *mdrv = dev_get_drvdata(dev);
>  
> -	if (!mdrv)
> -		return;
> -
>  	komeda_kms_detach(mdrv->kms);
>  
>  	if (pm_runtime_enabled(dev))
> @@ -46,10 +44,13 @@ static void komeda_unbind(struct device *dev)
>  
>  	dev_set_drvdata(dev, NULL);
>  	devm_kfree(dev, mdrv);
> +
> +	return 0;
>  }
>  
> -static int komeda_bind(struct device *dev)
> +static int komeda_platform_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct komeda_drv *mdrv;
>  	int err;
>  
> @@ -89,52 +90,7 @@ static int komeda_bind(struct device *dev)
>  free_mdrv:
>  	devm_kfree(dev, mdrv);
>  	return err;
> -}
> -
> -static const struct component_master_ops komeda_master_ops = {
> -	.bind	= komeda_bind,
> -	.unbind	= komeda_unbind,
> -};
>  
> -static void komeda_add_slave(struct device *master,
> -			     struct component_match **match,
> -			     struct device_node *np,
> -			     u32 port, u32 endpoint)
> -{
> -	struct device_node *remote;
> -
> -	remote = of_graph_get_remote_node(np, port, endpoint);
> -	if (remote) {
> -		drm_of_component_match_add(master, match, component_compare_of, remote);
> -		of_node_put(remote);
> -	}
> -}
> -
> -static int komeda_platform_probe(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	struct component_match *match = NULL;
> -	struct device_node *child;
> -
> -	if (!dev->of_node)
> -		return -ENODEV;
> -
> -	for_each_available_child_of_node(dev->of_node, child) {
> -		if (of_node_cmp(child->name, "pipeline") != 0)
> -			continue;
> -
> -		/* add connector */
> -		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 0);
> -		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 1);
> -	}
> -
> -	return component_master_add_with_match(dev, &komeda_master_ops, match);
> -}
> -
> -static int komeda_platform_remove(struct platform_device *pdev)
> -{
> -	component_master_del(&pdev->dev, &komeda_master_ops);
> -	return 0;
>  }
>  
>  static const struct of_device_id komeda_of_match[] = {
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 62dc64550793e..91cbcb6974021 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -305,10 +305,6 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
>  	if (err)
>  		goto cleanup_mode_config;
>  
> -	err = component_bind_all(mdev->dev, kms);
> -	if (err)
> -		goto cleanup_mode_config;
> -
>  	drm_mode_config_reset(drm);
>  
>  	err = devm_request_irq(drm->dev, mdev->irq,
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index 3a872c2920912..6ef6553263570 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -84,6 +84,9 @@ struct komeda_crtc {
>  
>  	/** @disable_done: this flip_done is for tracing the disable */
>  	struct completion *disable_done;
> +
> +	/** @encoder: encoder at the end of the pipeline */
> +	struct drm_encoder encoder;
>  };
>  
>  /**
> -- 
> 2.25.1
>

Code looks good and turns out swapping drm_bridge for component framework is not that painful. If you send v2
with the comments addressed I should be able to test it now and review the patch much sooner.

One issue I have observed from my testing of your patch is that on `rmmod komeda` we fail to disable the
interrupts after drm_kms_helper_poll_fini() call in komeda_kms_detach(), then we NULL the drm->dev_private
before we get an interrupt which causes komeda_kms_irq_handler() to access the NULL pointer. This is not
something caused by your patch, but if you want to test module removal I think you should be aware of this.

Best regards,
Liviu
  

Hi Liviu,

On 6/29/2023 3:21 PM, Liviu Dudau wrote:
> Hi Faiz,
>
> Thanks for the patch and for addressing what was at some moment on my "nice to
> improve / cleanup" list. Sorry for the delay in responding, I had to revive
> the bits of an old setup to be able to test this properly, with 2 encoders
> attached.
>
> On Wed, Jun 21, 2023 at 02:11:16PM +0530, Faiz Abbas wrote:
>> The Komeda driver always expects the remote connector node to initialize
>> an encoder. It uses the component aggregator framework which consists
>> of component->bind() calls used to initialize the remote encoder and attach
>> it to the crtc. This is different from other drm implementations which expect
>> the display driver to supply a crtc and connect an encoder to it.
> I think both approaches are valid in DRM. We don't want to remove the component
> framework because it is doing the wrong thing, but because we cannot use it
> with drivers that implement the drm_bridge API. Given that we usually pair with
> a component encoder that also implements a drm_bridge, dropping support for
> component framework should not affect the users of the driver.

Sounds good. I will update the commit message to emphasize the bridge API.

>> Remove all component framework calls from the komeda driver and declare and
>> attach an encoder inside komeda_crtc_add().
> Unfortunately you haven't removed all component framework calls. The hint for
> that is that you have not removed the #include <linux/component.h> line from
> any of the files. Specifically, komeda_kms_attach()/detach() still calls
> component_unbind_all() which will crash with your patch applied.

Good catch. Will remove that stuff in v2.

>> The remote connector driver has to implement the DRM bridge APIs which
>> can be used to glue the encoder to the remote connector.
>>
>> Signed-off-by: Faiz Abbas <faiz.abbas@arm.com>
>> ---
>>  .../gpu/drm/arm/display/komeda/komeda_crtc.c  | 22 +++++++-
>>  .../gpu/drm/arm/display/komeda/komeda_drv.c   | 56 ++-----------------
>>  .../gpu/drm/arm/display/komeda/komeda_kms.c   |  4 --
>>  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 +
>>  4 files changed, 30 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
>> index cea3fd5772b57..144736a69b0ee 100644
>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
>> @@ -12,6 +12,8 @@
>>  #include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_print.h>
>>  #include <drm/drm_vblank.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +#include <drm/drm_bridge.h>
>>  
>>  #include "komeda_dev.h"
>>  #include "komeda_kms.h"
>> @@ -612,9 +614,11 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>>  			   struct komeda_crtc *kcrtc)
>>  {
>>  	struct drm_crtc *crtc = &kcrtc->base;
>> +	struct drm_device *base = &kms->base;
>> +	struct drm_bridge *bridge;
>>  	int err;
>>  
>> -	err = drm_crtc_init_with_planes(&kms->base, crtc,
>> +	err = drm_crtc_init_with_planes(base, crtc,
>>  					get_crtc_primary(kms, kcrtc), NULL,
>>  					&komeda_crtc_funcs, NULL);
>>  	if (err)
>> @@ -626,6 +630,22 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>>  
>>  	drm_crtc_enable_color_mgmt(crtc, 0, true, KOMEDA_COLOR_LUT_SIZE);
> I would move this line after the bridges are being initialised, just in case in the future
> colour management will propagate some info down to the bridges.

Sure. I'll move it down.

.

.

.

>>  };
>>  
>>  /**
>> -- 
>> 2.25.1
>>
> Code looks good and turns out swapping drm_bridge for component framework is not that painful. If you send v2
> with the comments addressed I should be able to test it now and review the patch much sooner.
>
> One issue I have observed from my testing of your patch is that on `rmmod komeda` we fail to disable the
> interrupts after drm_kms_helper_poll_fini() call in komeda_kms_detach(), then we NULL the drm->dev_private
> before we get an interrupt which causes komeda_kms_irq_handler() to access the NULL pointer. This is not
> something caused by your patch, but if you want to test module removal I think you should be aware of this.

I'll keep this in mind while testing.

Thanks,
Faiz
  

Hi Mohammed,

On Tue, Jul 04, 2023 at 07:19:04PM +0530, Mohammad Faiz Abbas Rizvi wrote:
> Hi Liviu,
> 
> On 6/29/2023 3:21 PM, Liviu Dudau wrote:
> > Hi Faiz,
> >
> > Thanks for the patch and for addressing what was at some moment on my "nice to
> > improve / cleanup" list. Sorry for the delay in responding, I had to revive
> > the bits of an old setup to be able to test this properly, with 2 encoders
> > attached.
> >
> > On Wed, Jun 21, 2023 at 02:11:16PM +0530, Faiz Abbas wrote:
> >> The Komeda driver always expects the remote connector node to initialize
> >> an encoder. It uses the component aggregator framework which consists
> >> of component->bind() calls used to initialize the remote encoder and attach
> >> it to the crtc. This is different from other drm implementations which expect
> >> the display driver to supply a crtc and connect an encoder to it.
> > I think both approaches are valid in DRM. We don't want to remove the component
> > framework because it is doing the wrong thing, but because we cannot use it
> > with drivers that implement the drm_bridge API. Given that we usually pair with
> > a component encoder that also implements a drm_bridge, dropping support for
> > component framework should not affect the users of the driver.

Glad to see the patch - I think this is moving things in the right
direction.


While at it do you plan to support DRM_BRIDGE_ATTACH_NO_CONNECTOR?

I did not read the patch carefully but noticed this call with no flags:

> drm_bridge_attach(&kcrtc->encoder, bridge, NULL, 0);

To add support for DRM_BRIDGE_ATTACH_NO_CONNECTOR you may need to verify
that all relevant bridge drivers supports the flag.
You will be told at runtime but only if the relevant bridge driver is
used.

It can be done later and is obviously a separate patch.

	Sam
  

Hi Sam,

On 7/4/2023 10:26 PM, Sam Ravnborg wrote:
> Hi Mohammed,
> 
> On Tue, Jul 04, 2023 at 07:19:04PM +0530, Mohammad Faiz Abbas Rizvi wrote:
>> Hi Liviu,
>>
>> On 6/29/2023 3:21 PM, Liviu Dudau wrote:
>>> Hi Faiz,
>>>
>>> Thanks for the patch and for addressing what was at some moment on my "nice to
>>> improve / cleanup" list. Sorry for the delay in responding, I had to revive
>>> the bits of an old setup to be able to test this properly, with 2 encoders
>>> attached.
>>>
>>> On Wed, Jun 21, 2023 at 02:11:16PM +0530, Faiz Abbas wrote:
>>>> The Komeda driver always expects the remote connector node to initialize
>>>> an encoder. It uses the component aggregator framework which consists
>>>> of component->bind() calls used to initialize the remote encoder and attach
>>>> it to the crtc. This is different from other drm implementations which expect
>>>> the display driver to supply a crtc and connect an encoder to it.
>>> I think both approaches are valid in DRM. We don't want to remove the component
>>> framework because it is doing the wrong thing, but because we cannot use it
>>> with drivers that implement the drm_bridge API. Given that we usually pair with
>>> a component encoder that also implements a drm_bridge, dropping support for
>>> component framework should not affect the users of the driver.
> 
> Glad to see the patch - I think this is moving things in the right
> direction.
> 
> 
> While at it do you plan to support DRM_BRIDGE_ATTACH_NO_CONNECTOR?
> 
> I did not read the patch carefully but noticed this call with no flags:
> 
>> drm_bridge_attach(&kcrtc->encoder, bridge, NULL, 0);
> 
> To add support for DRM_BRIDGE_ATTACH_NO_CONNECTOR you may need to verify
> that all relevant bridge drivers supports the flag.
> You will be told at runtime but only if the relevant bridge driver is
> used.

Just from reading the documentation on this (https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L439) I gather this is useful for chains of multiple bridges where the connector is discovered at runtime? I'm not sure if we have a configuration like this with komeda here.

Also I'm not sure what it means for the CRTC driver to "support" this flag. Do I always call drm_bridge_attach() with it?

> 
> It can be done later and is obviously a separate patch.
> 

Sure though I cannot promise I can look into it anytime soon.

Thanks,
Faiz