[1/1] drm/sti: fix master bind bug for using component
diff mbox

Message ID 55A4D648.6060404@hisilicon.com
State New
Headers show

Commit Message

Xinwei Kong July 14, 2015, 9:28 a.m. UTC
From: Xinwei Kong <kong.kongxinwei@hisilicon.com>

This patch fix one bug which it can't call .bind function in
sti_hdmi.c and sti_hda.c file when changing the building sequence
(sti_hdmi.o?sti_hda.o) in Makefile file. This patch can prepare
it.

Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
---
 drivers/gpu/drm/sti/sti_tvout.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Benjamin Gaignard July 15, 2015, 10:19 a.m. UTC | #1
The build order in Makefile hasn't been change so the bug doesn't occur...

In addition of taking care of not changing build order in Makefile,
I would like to understand what is the expected behavior of component
framework when
master call component_bind_all() before any component_add() calls.

Should component bind function be called in component_add() ?
Is up to component to detect that master is already bounded ?

Russell can you tell us what to do in this case ?

Regards,
Benjamin

2015-07-14 11:28 GMT+02:00 Xinwei Kong <kong.kongxinwei@hisilicon.com>:
> From: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>
> This patch fix one bug which it can't call .bind function in
> sti_hdmi.c and sti_hda.c file when changing the building sequence
> (sti_hdmi.o?sti_hda.o) in Makefile file. This patch can prepare
> it.
>
> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> ---
>  drivers/gpu/drm/sti/sti_tvout.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
> index 5cc5311..293dfae 100644
> --- a/drivers/gpu/drm/sti/sti_tvout.c
> +++ b/drivers/gpu/drm/sti/sti_tvout.c
> @@ -683,7 +683,22 @@ static int compare_of(struct device *dev, void *data)
>
>  static int sti_tvout_master_bind(struct device *dev)
>  {
> -       return 0;
> +       struct sti_tvout *tvout = dev_get_drvdata(dev);
> +       struct drm_device *drm_dev;
> +       int ret;
> +
> +       if (!tvout->drm_dev) {
> +               DRM_ERROR("master bind is fail\n");
> +               return 0;
> +       }
> +
> +       drm_dev = tvout->drm_dev;
> +
> +       ret = component_bind_all(dev, drm_dev);
> +       if (ret)
> +               sti_tvout_destroy_encoders(tvout);
> +
> +       return ret;
>  }
>
>  static void sti_tvout_master_unbind(struct device *dev)
> --
> 1.9.1
>
Russell King - ARM Linux July 15, 2015, 10:42 a.m. UTC | #2
On Wed, Jul 15, 2015 at 12:19:01PM +0200, Benjamin Gaignard wrote:
> The build order in Makefile hasn't been change so the bug doesn't occur...
> 
> In addition of taking care of not changing build order in Makefile,
> I would like to understand what is the expected behavior of component
> framework when
> master call component_bind_all() before any component_add() calls.
> 
> Should component bind function be called in component_add() ?
> Is up to component to detect that master is already bounded ?
> 
> Russell can you tell us what to do in this case ?

I don't follow, and you certainly should never get into the situation
you're alluding to (where the master is already bound but a component
is not.)

The way this should work is:

- master and components register themselves into the component helper
  in a random order.
  - when the master registers, it gives the component helper a set of
    matches which it uses to determine which components are required.
- when the component helper determines that all components and the
  master have been registered, it calls the master bind function.
- the master bind function is responsible for the classical subsystem
  initialisation, calling component_bind_all() to cause the individual
  components bind() method to be called.

So, you should never _ever_ be in the situation where initcall ordering
matters, or where the master is already bound but a component hasn't
registered.
Benjamin Gaignard July 15, 2015, 1:17 p.m. UTC | #3
Thanks a lot Russell, I have now understand where was my mistake.


2015-07-15 12:42 GMT+02:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Wed, Jul 15, 2015 at 12:19:01PM +0200, Benjamin Gaignard wrote:
>> The build order in Makefile hasn't been change so the bug doesn't occur...
>>
>> In addition of taking care of not changing build order in Makefile,
>> I would like to understand what is the expected behavior of component
>> framework when
>> master call component_bind_all() before any component_add() calls.
>>
>> Should component bind function be called in component_add() ?
>> Is up to component to detect that master is already bounded ?
>>
>> Russell can you tell us what to do in this case ?
>
> I don't follow, and you certainly should never get into the situation
> you're alluding to (where the master is already bound but a component
> is not.)
>
> The way this should work is:
>
> - master and components register themselves into the component helper
>   in a random order.
>   - when the master registers, it gives the component helper a set of
>     matches which it uses to determine which components are required.
> - when the component helper determines that all components and the
>   master have been registered, it calls the master bind function.
> - the master bind function is responsible for the classical subsystem
>   initialisation, calling component_bind_all() to cause the individual
>   components bind() method to be called.
>
> So, you should never _ever_ be in the situation where initcall ordering
> matters, or where the master is already bound but a component hasn't
> registered.
>
> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
Xinwei Kong July 16, 2015, 1:13 a.m. UTC | #4
Thank you for Russel.

It is Right, when this sti_tvout (component) finish executing component ".bind"
function while sti_hdmi or sti_hda is not registered. the bug will occur .

this patch will prepare this bug by calling master .bind of sti_tvout after
sti_hdmi or sti_hda is register to finish binding sti_hdmi or sti_hda component,
however, it also slove to bring the "drm_dev" struct into the sti_hdmi or sti_hda.

Thank you
Xinwei

On 2015/7/15 21:17, Benjamin Gaignard wrote:
> Thanks a lot Russell, I have now understand where was my mistake.
> 
> 
> 2015-07-15 12:42 GMT+02:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> On Wed, Jul 15, 2015 at 12:19:01PM +0200, Benjamin Gaignard wrote:
>>> The build order in Makefile hasn't been change so the bug doesn't occur...
>>>
>>> In addition of taking care of not changing build order in Makefile,
>>> I would like to understand what is the expected behavior of component
>>> framework when
>>> master call component_bind_all() before any component_add() calls.
>>>
>>> Should component bind function be called in component_add() ?
>>> Is up to component to detect that master is already bounded ?
>>>
>>> Russell can you tell us what to do in this case ?
>>
>> I don't follow, and you certainly should never get into the situation
>> you're alluding to (where the master is already bound but a component
>> is not.)
>>
>> The way this should work is:
>>
>> - master and components register themselves into the component helper
>>   in a random order.
>>   - when the master registers, it gives the component helper a set of
>>     matches which it uses to determine which components are required.
>> - when the component helper determines that all components and the
>>   master have been registered, it calls the master bind function.
>> - the master bind function is responsible for the classical subsystem
>>   initialisation, calling component_bind_all() to cause the individual
>>   components bind() method to be called.
>>
>> So, you should never _ever_ be in the situation where initcall ordering
>> matters, or where the master is already bound but a component hasn't
>> registered.
>>
>> --
>> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
>> according to speedtest.net.
> 
> 
>
Benjamin Gaignard July 16, 2015, 7:17 a.m. UTC | #5
This patch isn't the good one, I send the fix here:
http://lists.freedesktop.org/archives/dri-devel/2015-July/086568.html

Regards,
Benjamin

2015-07-16 3:13 GMT+02:00 Xinwei Kong <kong.kongxinwei@hisilicon.com>:
> Thank you for Russel.
>
> It is Right, when this sti_tvout (component) finish executing component ".bind"
> function while sti_hdmi or sti_hda is not registered. the bug will occur .
>
> this patch will prepare this bug by calling master .bind of sti_tvout after
> sti_hdmi or sti_hda is register to finish binding sti_hdmi or sti_hda component,
> however, it also slove to bring the "drm_dev" struct into the sti_hdmi or sti_hda.
>
> Thank you
> Xinwei
>
> On 2015/7/15 21:17, Benjamin Gaignard wrote:
>> Thanks a lot Russell, I have now understand where was my mistake.
>>
>>
>> 2015-07-15 12:42 GMT+02:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>>> On Wed, Jul 15, 2015 at 12:19:01PM +0200, Benjamin Gaignard wrote:
>>>> The build order in Makefile hasn't been change so the bug doesn't occur...
>>>>
>>>> In addition of taking care of not changing build order in Makefile,
>>>> I would like to understand what is the expected behavior of component
>>>> framework when
>>>> master call component_bind_all() before any component_add() calls.
>>>>
>>>> Should component bind function be called in component_add() ?
>>>> Is up to component to detect that master is already bounded ?
>>>>
>>>> Russell can you tell us what to do in this case ?
>>>
>>> I don't follow, and you certainly should never get into the situation
>>> you're alluding to (where the master is already bound but a component
>>> is not.)
>>>
>>> The way this should work is:
>>>
>>> - master and components register themselves into the component helper
>>>   in a random order.
>>>   - when the master registers, it gives the component helper a set of
>>>     matches which it uses to determine which components are required.
>>> - when the component helper determines that all components and the
>>>   master have been registered, it calls the master bind function.
>>> - the master bind function is responsible for the classical subsystem
>>>   initialisation, calling component_bind_all() to cause the individual
>>>   components bind() method to be called.
>>>
>>> So, you should never _ever_ be in the situation where initcall ordering
>>> matters, or where the master is already bound but a component hasn't
>>> registered.
>>>
>>> --
>>> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
>>> according to speedtest.net.
>>
>>
>>
>
Xinwei Kong July 16, 2015, 9:06 a.m. UTC | #6
hi ben:

your patch is ok, i don't know your hardware how to use. In this first
code why use that style dts? If you detail research our patch, you will
find that it can compatiable your fixing dts.

If you don't approve our patch better, I will be glad to see you to slove
this bug

Thank you
Xinwei



On 2015/7/16 15:17, Benjamin Gaignard wrote:
> This patch isn't the good one, I send the fix here:
> http://lists.freedesktop.org/archives/dri-devel/2015-July/086568.html
> 
> Regards,
> Benjamin
> 
> 2015-07-16 3:13 GMT+02:00 Xinwei Kong <kong.kongxinwei@hisilicon.com>:
>> Thank you for Russel.
>>
>> It is Right, when this sti_tvout (component) finish executing component ".bind"
>> function while sti_hdmi or sti_hda is not registered. the bug will occur .
>>
>> this patch will prepare this bug by calling master .bind of sti_tvout after
>> sti_hdmi or sti_hda is register to finish binding sti_hdmi or sti_hda component,
>> however, it also slove to bring the "drm_dev" struct into the sti_hdmi or sti_hda.
>>
>> Thank you
>> Xinwei
>>
>> On 2015/7/15 21:17, Benjamin Gaignard wrote:
>>> Thanks a lot Russell, I have now understand where was my mistake.
>>>
>>>
>>> 2015-07-15 12:42 GMT+02:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>>>> On Wed, Jul 15, 2015 at 12:19:01PM +0200, Benjamin Gaignard wrote:
>>>>> The build order in Makefile hasn't been change so the bug doesn't occur...
>>>>>
>>>>> In addition of taking care of not changing build order in Makefile,
>>>>> I would like to understand what is the expected behavior of component
>>>>> framework when
>>>>> master call component_bind_all() before any component_add() calls.
>>>>>
>>>>> Should component bind function be called in component_add() ?
>>>>> Is up to component to detect that master is already bounded ?
>>>>>
>>>>> Russell can you tell us what to do in this case ?
>>>>
>>>> I don't follow, and you certainly should never get into the situation
>>>> you're alluding to (where the master is already bound but a component
>>>> is not.)
>>>>
>>>> The way this should work is:
>>>>
>>>> - master and components register themselves into the component helper
>>>>   in a random order.
>>>>   - when the master registers, it gives the component helper a set of
>>>>     matches which it uses to determine which components are required.
>>>> - when the component helper determines that all components and the
>>>>   master have been registered, it calls the master bind function.
>>>> - the master bind function is responsible for the classical subsystem
>>>>   initialisation, calling component_bind_all() to cause the individual
>>>>   components bind() method to be called.
>>>>
>>>> So, you should never _ever_ be in the situation where initcall ordering
>>>> matters, or where the master is already bound but a component hasn't
>>>> registered.
>>>>
>>>> --
>>>> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
>>>> according to speedtest.net.
>>>
>>>
>>>
>>
> 
> 
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
index 5cc5311..293dfae 100644
--- a/drivers/gpu/drm/sti/sti_tvout.c
+++ b/drivers/gpu/drm/sti/sti_tvout.c
@@ -683,7 +683,22 @@  static int compare_of(struct device *dev, void *data)

 static int sti_tvout_master_bind(struct device *dev)
 {
-	return 0;
+	struct sti_tvout *tvout = dev_get_drvdata(dev);
+	struct drm_device *drm_dev;
+	int ret;
+
+	if (!tvout->drm_dev) {
+		DRM_ERROR("master bind is fail\n");
+		return 0;
+	}
+
+	drm_dev = tvout->drm_dev;
+
+	ret = component_bind_all(dev, drm_dev);
+	if (ret)
+		sti_tvout_destroy_encoders(tvout);
+
+	return ret;
 }

 static void sti_tvout_master_unbind(struct device *dev)