[PATCH 0/2] OMAPDSS: small fixes for 3.3 rc

[PATCH 0/2] OMAPDSS: small fixes for 3.3 rc

[Tomi Valkeinen]

Hi,

Two small fixes for omapdss.

Florian, if the patches are ok, I guess it's easier if you just apply these
patches from email, instead of me sending a pull request?

 Tomi

Tomi Valkeinen (2):
  OMAPDSS: panel-dvi: Add Kconfig dependency on I2C
  OMAPDSS: APPLY: make ovl_enable/disable synchronous

 drivers/video/omap2/displays/Kconfig |    2 +-
 drivers/video/omap2/dss/apply.c      |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] OMAPDSS: panel-dvi: Add Kconfig dependency on I2C

[Tomi Valkeinen]

panel-dvi uses i2c, but the Kconfig didn't have dependency on I2C. Add
it.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/displays/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/video/omap2/displays/Kconfig b/drivers/video/omap2/displays/Kconfig
index 74d29b5..408a992 100644
--- a/drivers/video/omap2/displays/Kconfig
+++ b/drivers/video/omap2/displays/Kconfig
@@ -12,7 +12,7 @@ config PANEL_GENERIC_DPI
 
 config PANEL_DVI
 	tristate "DVI output"
-	depends on OMAP2_DSS_DPI
+	depends on OMAP2_DSS_DPI && I2C
 	help
 	  Driver for external monitors, connected via DVI. The driver uses i2c
 	  to read EDID information from the monitor.
-- 
1.7.4.1

Re: [PATCH 1/2] OMAPDSS: panel-dvi: Add Kconfig dependency on I2C

[Florian Tobias Schandinat]

On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
> panel-dvi uses i2c, but the Kconfig didn't have dependency on I2C. Add
> it.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/omap2/displays/Kconfig |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/omap2/displays/Kconfig b/drivers/video/omap2/displays/Kconfig
> index 74d29b5..408a992 100644
> --- a/drivers/video/omap2/displays/Kconfig
> +++ b/drivers/video/omap2/displays/Kconfig
> @@ -12,7 +12,7 @@ config PANEL_GENERIC_DPI
>  
>  config PANEL_DVI
>  	tristate "DVI output"
> -	depends on OMAP2_DSS_DPI
> +	depends on OMAP2_DSS_DPI && I2C

It's just a matter of taste, but are you sure you want to "depend" on it and not
"select" it? Other drivers tend to use select for I2C, for me it doesn't really
matter.


Best regards,

Florian Tobias Schandinat

>  	help
>  	  Driver for external monitors, connected via DVI. The driver uses i2c
>  	  to read EDID information from the monitor.

Re: [PATCH 1/2] OMAPDSS: panel-dvi: Add Kconfig dependency on I2C

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

On Wed, 2012-02-29 at 10:03 +0000, Florian Tobias Schandinat wrote:
> On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
> > panel-dvi uses i2c, but the Kconfig didn't have dependency on I2C. Add
> > it.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> >  drivers/video/omap2/displays/Kconfig |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/video/omap2/displays/Kconfig b/drivers/video/omap2/displays/Kconfig
> > index 74d29b5..408a992 100644
> > --- a/drivers/video/omap2/displays/Kconfig
> > +++ b/drivers/video/omap2/displays/Kconfig
> > @@ -12,7 +12,7 @@ config PANEL_GENERIC_DPI
> >  
> >  config PANEL_DVI
> >  	tristate "DVI output"
> > -	depends on OMAP2_DSS_DPI
> > +	depends on OMAP2_DSS_DPI && I2C
> 
> It's just a matter of taste, but are you sure you want to "depend" on it and not
> "select" it? Other drivers tend to use select for I2C, for me it doesn't really
> matter.

Well, I'd like to "select", but I don't think that's correct. From
Documentation/kbuild/kconfig-language.txt:

        In general use select only for non-visible symbols
        (no prompts anywhere) and for symbols with no dependencies.

But I do see quite many selects for I2C, so I'm not sure if all those
are wrong, or has it just been decided that I2C is a valid target for
select.

Using depend is in line with the other panel drivers in the same
directory. I've been thinking about the same thing from time to time,
and I'd rather select I2C, SPI and BACKLIGHT_CLASS_DEVICE than use
depend.

But, for example, using select to BACKLIGHT_CLASS_DEVICE is broken: if I
change a panel driver to select BACKLIGHT_CLASS_DEVICE, but then I
manually disable CONFIG_BACKLIGHT_LCD_SUPPORT (which the
BACKLIGHT_CLASS_DEVICE depends on) from the kernel config, this leads to
BACKLIGHT_CLASS_DEVICE being enabled, but CONFIG_BACKLIGHT_LCD_SUPPORT
being disabled, which is clearly broken.

So... As I see it, depending is a bit awkward, but it's correct.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] OMAPDSS: panel-dvi: Add Kconfig dependency on I2C

[Florian Tobias Schandinat]

Hi Tomi,

On 02/29/2012 10:21 AM, Tomi Valkeinen wrote:
> On Wed, 2012-02-29 at 10:03 +0000, Florian Tobias Schandinat wrote:
>> On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
>>> panel-dvi uses i2c, but the Kconfig didn't have dependency on I2C. Add
>>> it.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>  drivers/video/omap2/displays/Kconfig |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/video/omap2/displays/Kconfig b/drivers/video/omap2/displays/Kconfig
>>> index 74d29b5..408a992 100644
>>> --- a/drivers/video/omap2/displays/Kconfig
>>> +++ b/drivers/video/omap2/displays/Kconfig
>>> @@ -12,7 +12,7 @@ config PANEL_GENERIC_DPI
>>>  
>>>  config PANEL_DVI
>>>  	tristate "DVI output"
>>> -	depends on OMAP2_DSS_DPI
>>> +	depends on OMAP2_DSS_DPI && I2C
>>
>> It's just a matter of taste, but are you sure you want to "depend" on it and not
>> "select" it? Other drivers tend to use select for I2C, for me it doesn't really
>> matter.
> 
> Well, I'd like to "select", but I don't think that's correct. From
> Documentation/kbuild/kconfig-language.txt:
> 
>         In general use select only for non-visible symbols
>         (no prompts anywhere) and for symbols with no dependencies.
> 
> But I do see quite many selects for I2C, so I'm not sure if all those
> are wrong, or has it just been decided that I2C is a valid target for
> select.

I'd say that the above is only a guideline but not absolute rule. For I2C I'd
argue that I probably wouldn't even know that my hardware has it if I weren't
the one writing the driver using it.
But it's true that select should be used with care as it is much easier to get
the config messed up by using it.

> Using depend is in line with the other panel drivers in the same
> directory. I've been thinking about the same thing from time to time,
> and I'd rather select I2C, SPI and BACKLIGHT_CLASS_DEVICE than use
> depend.
> 
> But, for example, using select to BACKLIGHT_CLASS_DEVICE is broken: if I
> change a panel driver to select BACKLIGHT_CLASS_DEVICE, but then I
> manually disable CONFIG_BACKLIGHT_LCD_SUPPORT (which the
> BACKLIGHT_CLASS_DEVICE depends on) from the kernel config, this leads to
> BACKLIGHT_CLASS_DEVICE being enabled, but CONFIG_BACKLIGHT_LCD_SUPPORT
> being disabled, which is clearly broken.

Yes, as far as I understand the problem here is that we don't have any mechanism
to handle transitive dependencies (probably because we don't want it). So if one
wants to do select something I think the right way to do it would be to inherit
all its select/depend statements in the option.

> So... As I see it, depending is a bit awkward, but it's correct.

Yes, it certainly is correct.


Best regards,

Florian Tobias Schandinat

[PATCH 2/2] OMAPDSS: APPLY: make ovl_enable/disable synchronous

[Tomi Valkeinen]

ovl->enable/disable are meant to be synchronous so that they can handle
the configuration of fifo sizes. The current kernel doesn't configure
fifo sizes yet, and so the code doesn't need to block to function (from
omapdss driver's perspective).

However, for the users of omapdss a non-blocking ovl->disable is
confusing, because they don't know when the memory area is not used
any more.

Furthermore, when the fifo size configuration is added in the next merge
window, the change from non-blocking to blocking could cause side
effects to the users of omapdss. So by making the functions block
already will keep them behaving in the same manner.

And, while not the main purpose of this patch, this will also remove the
compile warning:

drivers/video/omap2/dss/apply.c:350: warning:
'wait_pending_extra_info_updates' defined but not used

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/apply.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 052dc87..87b3e25 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -1276,6 +1276,9 @@ int dss_ovl_enable(struct omap_overlay *ovl)
 
 	spin_unlock_irqrestore(&data_lock, flags);
 
+	/* wait for overlay to be enabled */
+	wait_pending_extra_info_updates();
+
 	mutex_unlock(&apply_lock);
 
 	return 0;
@@ -1313,6 +1316,9 @@ int dss_ovl_disable(struct omap_overlay *ovl)
 
 	spin_unlock_irqrestore(&data_lock, flags);
 
+	/* wait for the overlay to be disabled */
+	wait_pending_extra_info_updates();
+
 	mutex_unlock(&apply_lock);
 
 	return 0;
-- 
1.7.4.1

Re: [PATCH 2/2] OMAPDSS: APPLY: make ovl_enable/disable synchronous

[Florian Tobias Schandinat]

On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
> ovl->enable/disable are meant to be synchronous so that they can handle
> the configuration of fifo sizes. The current kernel doesn't configure
> fifo sizes yet, and so the code doesn't need to block to function (from
> omapdss driver's perspective).
> 
> However, for the users of omapdss a non-blocking ovl->disable is
> confusing, because they don't know when the memory area is not used
> any more.
> 
> Furthermore, when the fifo size configuration is added in the next merge
> window, the change from non-blocking to blocking could cause side
> effects to the users of omapdss. So by making the functions block
> already will keep them behaving in the same manner.

Is there any difference to doing it now?
I agree that this should be fixed but if we can't avoid breaking users I'd
prefer to break them in a merge window not in late rc stage. Or did we introduce
these functions just in the last merge window?


Best regards,

Florian Tobias Schandinat

> 
> And, while not the main purpose of this patch, this will also remove the
> compile warning:
> 
> drivers/video/omap2/dss/apply.c:350: warning:
> 'wait_pending_extra_info_updates' defined but not used
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/omap2/dss/apply.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index 052dc87..87b3e25 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -1276,6 +1276,9 @@ int dss_ovl_enable(struct omap_overlay *ovl)
>  
>  	spin_unlock_irqrestore(&data_lock, flags);
>  
> +	/* wait for overlay to be enabled */
> +	wait_pending_extra_info_updates();
> +
>  	mutex_unlock(&apply_lock);
>  
>  	return 0;
> @@ -1313,6 +1316,9 @@ int dss_ovl_disable(struct omap_overlay *ovl)
>  
>  	spin_unlock_irqrestore(&data_lock, flags);
>  
> +	/* wait for the overlay to be disabled */
> +	wait_pending_extra_info_updates();
> +
>  	mutex_unlock(&apply_lock);
>  
>  	return 0;

Re: [PATCH 2/2] OMAPDSS: APPLY: make ovl_enable/disable synchronous

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

On Wed, 2012-02-29 at 10:13 +0000, Florian Tobias Schandinat wrote:
> On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
> > ovl->enable/disable are meant to be synchronous so that they can handle
> > the configuration of fifo sizes. The current kernel doesn't configure
> > fifo sizes yet, and so the code doesn't need to block to function (from
> > omapdss driver's perspective).
> > 
> > However, for the users of omapdss a non-blocking ovl->disable is
> > confusing, because they don't know when the memory area is not used
> > any more.
> > 
> > Furthermore, when the fifo size configuration is added in the next merge
> > window, the change from non-blocking to blocking could cause side
> > effects to the users of omapdss. So by making the functions block
> > already will keep them behaving in the same manner.
> 
> Is there any difference to doing it now?
> I agree that this should be fixed but if we can't avoid breaking users I'd
> prefer to break them in a merge window not in late rc stage. Or did we introduce
> these functions just in the last merge window?

Yes, these were introduced in the merge window. And I explicitly said
the functions are blocking so that they can perform their job. And just
to clarify, the functions already use a mutex, so in that sense they are
blocking. They just don't currently wait until the HW has finished with
the overlay.

The problem was raised by Rob Clark, who's writing the omapdrm driver,
as he doesn't have a way to ensure that the overlay has been truly
disabled and the memory is is no longer in use.

(I forgot to cc him for the patch, adding him now).

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] OMAPDSS: APPLY: make ovl_enable/disable synchronous

[Florian Tobias Schandinat]

Hi Tomi,

On 02/29/2012 10:30 AM, Tomi Valkeinen wrote:
> On Wed, 2012-02-29 at 10:13 +0000, Florian Tobias Schandinat wrote:
>> On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
>>> ovl->enable/disable are meant to be synchronous so that they can handle
>>> the configuration of fifo sizes. The current kernel doesn't configure
>>> fifo sizes yet, and so the code doesn't need to block to function (from
>>> omapdss driver's perspective).
>>>
>>> However, for the users of omapdss a non-blocking ovl->disable is
>>> confusing, because they don't know when the memory area is not used
>>> any more.
>>>
>>> Furthermore, when the fifo size configuration is added in the next merge
>>> window, the change from non-blocking to blocking could cause side
>>> effects to the users of omapdss. So by making the functions block
>>> already will keep them behaving in the same manner.
>>
>> Is there any difference to doing it now?
>> I agree that this should be fixed but if we can't avoid breaking users I'd
>> prefer to break them in a merge window not in late rc stage. Or did we introduce
>> these functions just in the last merge window?
> 
> Yes, these were introduced in the merge window. And I explicitly said
> the functions are blocking so that they can perform their job. And just
> to clarify, the functions already use a mutex, so in that sense they are
> blocking. They just don't currently wait until the HW has finished with
> the overlay.

okay, than I'll apply this patch as is. I was just worried about asking Linus to
pull something that is labled as "Breaks existing users" now, but that doesn't
look like an issue here.


Best regards,

Florian Tobias Schandinat

> 
> The problem was raised by Rob Clark, who's writing the omapdrm driver,
> as he doesn't have a way to ensure that the overlay has been truly
> disabled and the memory is is no longer in use.
> 
> (I forgot to cc him for the patch, adding him now).
> 
>  Tomi
> 

Re: [PATCH 2/2] OMAPDSS: APPLY: make ovl_enable/disable synchronous

[Rob Clark]

On Wed, Feb 29, 2012 at 4:48 AM, Florian Tobias Schandinat
<FlorianSchandinat@gmx.de> wrote:
> Hi Tomi,
>
> On 02/29/2012 10:30 AM, Tomi Valkeinen wrote:
>> On Wed, 2012-02-29 at 10:13 +0000, Florian Tobias Schandinat wrote:
>>> On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
>>>> ovl->enable/disable are meant to be synchronous so that they can handle
>>>> the configuration of fifo sizes. The current kernel doesn't configure
>>>> fifo sizes yet, and so the code doesn't need to block to function (from
>>>> omapdss driver's perspective).
>>>>
>>>> However, for the users of omapdss a non-blocking ovl->disable is
>>>> confusing, because they don't know when the memory area is not used
>>>> any more.
>>>>
>>>> Furthermore, when the fifo size configuration is added in the next merge
>>>> window, the change from non-blocking to blocking could cause side
>>>> effects to the users of omapdss. So by making the functions block
>>>> already will keep them behaving in the same manner.
>>>
>>> Is there any difference to doing it now?
>>> I agree that this should be fixed but if we can't avoid breaking users I'd
>>> prefer to break them in a merge window not in late rc stage. Or did we introduce
>>> these functions just in the last merge window?
>>
>> Yes, these were introduced in the merge window. And I explicitly said
>> the functions are blocking so that they can perform their job. And just
>> to clarify, the functions already use a mutex, so in that sense they are
>> blocking. They just don't currently wait until the HW has finished with
>> the overlay.
>
> okay, than I'll apply this patch as is. I was just worried about asking Linus to
> pull something that is labled as "Breaks existing users" now, but that doesn't
> look like an issue here.
>

I don't expect this change should break any existing users.. I think
it is safe to call this a bug fix

BR,
-R

> Best regards,
>
> Florian Tobias Schandinat
>
>>
>> The problem was raised by Rob Clark, who's writing the omapdrm driver,
>> as he doesn't have a way to ensure that the overlay has been truly
>> disabled and the memory is is no longer in use.
>>
>> (I forgot to cc him for the patch, adding him now).
>>
>>  Tomi
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] OMAPDSS: small fixes for 3.3 rc

[Florian Tobias Schandinat]

Hi Tomi,

On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
> Hi,
> 
> Two small fixes for omapdss.
> 
> Florian, if the patches are ok, I guess it's easier if you just apply these
> patches from email, instead of me sending a pull request?

Yes, that's correct as those are only a few patches and as we are already in
late -rc's (and therefore I guess, I should take a better look at what I apply).
I'll add some comments to your patches in separate mails.


Best regards,

Florian Tobias Schandinat

> 
>  Tomi
> 
> Tomi Valkeinen (2):
>   OMAPDSS: panel-dvi: Add Kconfig dependency on I2C
>   OMAPDSS: APPLY: make ovl_enable/disable synchronous
> 
>  drivers/video/omap2/displays/Kconfig |    2 +-
>  drivers/video/omap2/dss/apply.c      |    6 ++++++
>  2 files changed, 7 insertions(+), 1 deletions(-)
> 

Re: [PATCH 0/2] OMAPDSS: small fixes for 3.3 rc

[Florian Tobias Schandinat]

On 02/29/2012 08:48 AM, Tomi Valkeinen wrote:
> Hi,
> 
> Two small fixes for omapdss.

Applied both.


Thanks,

Florian Tobias Schandinat

> 
> Florian, if the patches are ok, I guess it's easier if you just apply these
> patches from email, instead of me sending a pull request?
> 
>  Tomi
> 
> Tomi Valkeinen (2):
>   OMAPDSS: panel-dvi: Add Kconfig dependency on I2C
>   OMAPDSS: APPLY: make ovl_enable/disable synchronous
> 
>  drivers/video/omap2/displays/Kconfig |    2 +-
>  drivers/video/omap2/dss/apply.c      |    6 ++++++
>  2 files changed, 7 insertions(+), 1 deletions(-)
>