RE: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
Hi Rob,
On Fri, Jan 25, 2013 at 20:22:55, Rob Clark wrote:
On Fri, Jan 25, 2013 at 8:15 AM, Mohammed, Afzal af...@ti.com wrote:
It's not about being simple, but not doing the wrong way, here you are
relying on a platform specific clock in a driver, think about the case
where same IP is used on another platform. Either way it is not a good
thing to handle platform specific details (about disp_clk) in driver.
Right, but I was trying to understand what was the benefit that the
added complexity is. I didn't realize on davinci that you are limited
Here I am referring to usage of disp_clk,
Driver is not supposed to do platform hacks - here you are trying to
configure something (PLL) in your driver which is not part of LCDC IP.
And LCDC IP is not aware of PLL which is a platform specific matter
(existent only in AM335x), it is only aware of functional clock.
You can set the rate on fck (functional clock) in AM335x using my patch,
ARM: AM33XX: clock: SET_RATE_PARENT in lcd path, and there
would not be any need for driver to be aware of platform specific PLL.
+ priv-clk = clk_get(dev-dev, fck);
+ priv-disp_clk = clk_get(dev-dev, dpll_disp_ck);
at the moment all this discussion is a bit moot. I'd propose leaving
the driver as it is for now, because that at least makes it useful on
am33xx. And when CCF and davinci have the needed support in place,
Let's forget about leveraging CCF in driver, but sane solution w.r.t PLL
configuration would be to do as mentioned above.
Regards
Afzal
N�r��yb�X��ǧv�^�){.n�+{��f��{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj��!�i
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Mon, Jan 28, 2013 at 3:56 AM, Mohammed, Afzal af...@ti.com wrote: Hi Rob, On Fri, Jan 25, 2013 at 20:22:55, Rob Clark wrote: On Fri, Jan 25, 2013 at 8:15 AM, Mohammed, Afzal af...@ti.com wrote: It's not about being simple, but not doing the wrong way, here you are relying on a platform specific clock in a driver, think about the case where same IP is used on another platform. Either way it is not a good thing to handle platform specific details (about disp_clk) in driver. Right, but I was trying to understand what was the benefit that the added complexity is. I didn't realize on davinci that you are limited Here I am referring to usage of disp_clk, Driver is not supposed to do platform hacks - here you are trying to configure something (PLL) in your driver which is not part of LCDC IP. And LCDC IP is not aware of PLL which is a platform specific matter (existent only in AM335x), it is only aware of functional clock. You can set the rate on fck (functional clock) in AM335x using my patch, ARM: AM33XX: clock: SET_RATE_PARENT in lcd path, and there would not be any need for driver to be aware of platform specific PLL. right, but I think it would be better to just make another patch that changes tilcdc to just set rate on fck after that patch is merged. I mean, I'd rather have the driver at least usable on AM33xx until then, rather than broken for everyone. BR, -R + priv-clk = clk_get(dev-dev, fck); + priv-disp_clk = clk_get(dev-dev, dpll_disp_ck); at the moment all this discussion is a bit moot. I'd propose leaving the driver as it is for now, because that at least makes it useful on am33xx. And when CCF and davinci have the needed support in place, Let's forget about leveraging CCF in driver, but sane solution w.r.t PLL configuration would be to do as mentioned above. Regards Afzal -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
Hi Rob,
On Wed, Jan 23, 2013 at 04:06:22, Rob Clark wrote:
A simple DRM/KMS driver for the TI LCD Controller found in various
smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the
+void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
+ /* in raster mode, minimum divisor is 2: */
+ ret = clk_set_rate(priv-disp_clk, crtc-mode.clock * 1000 * 2);
These things can better be handled with divider clock having a
minimum value (being discussed with Mike on how exactly it should
be) and instead of setting rate over a platform specific clock,
you can set rate over lcd clock using SET_RATE_PARENT at platform
level, more below,
+ /* Configure the LCD clock divisor. */
+ tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(div) |
+ LCDC_RASTER_MODE);
+
+ if (priv-rev == 2)
+ tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+ LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+ LCDC_V2_CORE_CLK_EN);
Mike was suggesting to model the above using gate/divider/composite
clocks of CCF in the FB driver.
+ priv-clk = clk_get(dev-dev, fck);
+ if (IS_ERR(priv-clk)) {
+ dev_err(dev-dev, failed to get functional clock\n);
+ ret = -ENODEV;
+ goto fail;
+ }
+
+ priv-disp_clk = clk_get(dev-dev, dpll_disp_ck);
+ if (IS_ERR(priv-clk)) {
+ dev_err(dev-dev, failed to get display clock\n);
+ ret = -ENODEV;
+ goto fail;
+ }
dpll_disp_ck is a platform specific matter, driver should not
be handling platform specifics. And this won't work on DaVinci,
you can probably make use of
my series [PATCH v2 0/4] ARM: AM335x: LCDC platform support
or something similar to overcome this.
Regards
Afzal
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Fri, Jan 25, 2013 at 7:19 AM, Mohammed, Afzal af...@ti.com wrote:
Hi Rob,
On Wed, Jan 23, 2013 at 04:06:22, Rob Clark wrote:
A simple DRM/KMS driver for the TI LCD Controller found in various
smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the
+void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
+ /* in raster mode, minimum divisor is 2: */
+ ret = clk_set_rate(priv-disp_clk, crtc-mode.clock * 1000 * 2);
These things can better be handled with divider clock having a
minimum value (being discussed with Mike on how exactly it should
be) and instead of setting rate over a platform specific clock,
you can set rate over lcd clock using SET_RATE_PARENT at platform
level, more below,
I looked at that patch you were proposing for da8xx-fb.. to be
honest, it did not seem simpler to me, so I was sort of failing to see
the benefit..
+ /* Configure the LCD clock divisor. */
+ tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(div) |
+ LCDC_RASTER_MODE);
+
+ if (priv-rev == 2)
+ tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+ LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+ LCDC_V2_CORE_CLK_EN);
Mike was suggesting to model the above using gate/divider/composite
clocks of CCF in the FB driver.
+ priv-clk = clk_get(dev-dev, fck);
+ if (IS_ERR(priv-clk)) {
+ dev_err(dev-dev, failed to get functional clock\n);
+ ret = -ENODEV;
+ goto fail;
+ }
+
+ priv-disp_clk = clk_get(dev-dev, dpll_disp_ck);
+ if (IS_ERR(priv-clk)) {
+ dev_err(dev-dev, failed to get display clock\n);
+ ret = -ENODEV;
+ goto fail;
+ }
dpll_disp_ck is a platform specific matter, driver should not
be handling platform specifics. And this won't work on DaVinci,
you can probably make use of
meaning the clock has a different name on davinci, or? Presumably
there must be some clock generating the pixel clock for the display,
but I confess to not being too familiar with the details on davinci..
BR,
-R
my series [PATCH v2 0/4] ARM: AM335x: LCDC platform support
or something similar to overcome this.
Regards
Afzal
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
Hi Rob,
On Fri, Jan 25, 2013 at 19:29:40, Rob Clark wrote:
On Fri, Jan 25, 2013 at 7:19 AM, Mohammed, Afzal af...@ti.com wrote:
On Wed, Jan 23, 2013 at 04:06:22, Rob Clark wrote:
A simple DRM/KMS driver for the TI LCD Controller found in various
smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the
+void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
+ /* in raster mode, minimum divisor is 2: */
+ ret = clk_set_rate(priv-disp_clk, crtc-mode.clock * 1000 * 2);
These things can better be handled with divider clock having a
minimum value (being discussed with Mike on how exactly it should
be) and instead of setting rate over a platform specific clock,
you can set rate over lcd clock using SET_RATE_PARENT at platform
level, more below,
I looked at that patch you were proposing for da8xx-fb.. to be
honest, it did not seem simpler to me, so I was sort of failing to see
the benefit..
It's not about being simple, but not doing the wrong way, here you are
relying on a platform specific clock in a driver, think about the case
where same IP is used on another platform. Either way it is not a good
thing to handle platform specific details (about disp_clk) in driver.
And Mike mentioned that one of design goals of CCF is to model these
kinds of clocks in IP's.
+ /* Configure the LCD clock divisor. */
+ tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(div) |
+ LCDC_RASTER_MODE);
+
+ if (priv-rev == 2)
+ tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+ LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+ LCDC_V2_CORE_CLK_EN);
Mike was suggesting to model the above using gate/divider/composite
clocks of CCF in the FB driver.
+ priv-clk = clk_get(dev-dev, fck);
+ if (IS_ERR(priv-clk)) {
+ dev_err(dev-dev, failed to get functional clock\n);
+ ret = -ENODEV;
+ goto fail;
+ }
+
+ priv-disp_clk = clk_get(dev-dev, dpll_disp_ck);
+ if (IS_ERR(priv-clk)) {
+ dev_err(dev-dev, failed to get display clock\n);
+ ret = -ENODEV;
+ goto fail;
+ }
dpll_disp_ck is a platform specific matter, driver should not
be handling platform specifics. And this won't work on DaVinci,
you can probably make use of
meaning the clock has a different name on davinci, or? Presumably
there must be some clock generating the pixel clock for the display,
but I confess to not being too familiar with the details on davinci..
The only option for the driver in DaVinci is to configure clock rate
using the divider of LCDC IP, it has fck, which gives a rate
decided by its ancestors.
Regards
Afzal
N�r��yb�X��ǧv�^�){.n�+{��f��{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj��!�i
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Fri, Jan 25, 2013 at 8:15 AM, Mohammed, Afzal af...@ti.com wrote:
Hi Rob,
On Fri, Jan 25, 2013 at 19:29:40, Rob Clark wrote:
On Fri, Jan 25, 2013 at 7:19 AM, Mohammed, Afzal af...@ti.com wrote:
On Wed, Jan 23, 2013 at 04:06:22, Rob Clark wrote:
A simple DRM/KMS driver for the TI LCD Controller found in various
smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the
+void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
+ /* in raster mode, minimum divisor is 2: */
+ ret = clk_set_rate(priv-disp_clk, crtc-mode.clock * 1000 * 2);
These things can better be handled with divider clock having a
minimum value (being discussed with Mike on how exactly it should
be) and instead of setting rate over a platform specific clock,
you can set rate over lcd clock using SET_RATE_PARENT at platform
level, more below,
I looked at that patch you were proposing for da8xx-fb.. to be
honest, it did not seem simpler to me, so I was sort of failing to see
the benefit..
It's not about being simple, but not doing the wrong way, here you are
relying on a platform specific clock in a driver, think about the case
where same IP is used on another platform. Either way it is not a good
thing to handle platform specific details (about disp_clk) in driver.
Right, but I was trying to understand what was the benefit that the
added complexity is. I didn't realize on davinci that you are limited
to just setting divider values (which is going to drastically limit
the timings you can generate, although maybe not an issue if all you
support is a fixed lcd panel).
And Mike mentioned that one of design goals of CCF is to model these
kinds of clocks in IP's.
+ /* Configure the LCD clock divisor. */
+ tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(div) |
+ LCDC_RASTER_MODE);
+
+ if (priv-rev == 2)
+ tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+ LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+ LCDC_V2_CORE_CLK_EN);
Mike was suggesting to model the above using gate/divider/composite
clocks of CCF in the FB driver.
+ priv-clk = clk_get(dev-dev, fck);
+ if (IS_ERR(priv-clk)) {
+ dev_err(dev-dev, failed to get functional clock\n);
+ ret = -ENODEV;
+ goto fail;
+ }
+
+ priv-disp_clk = clk_get(dev-dev, dpll_disp_ck);
+ if (IS_ERR(priv-clk)) {
+ dev_err(dev-dev, failed to get display clock\n);
+ ret = -ENODEV;
+ goto fail;
+ }
dpll_disp_ck is a platform specific matter, driver should not
be handling platform specifics. And this won't work on DaVinci,
you can probably make use of
meaning the clock has a different name on davinci, or? Presumably
there must be some clock generating the pixel clock for the display,
but I confess to not being too familiar with the details on davinci..
The only option for the driver in DaVinci is to configure clock rate
using the divider of LCDC IP, it has fck, which gives a rate
decided by its ancestors.
Well, from looking at the other patch series it seems CCF doesn't
support minimum divider yet. And davinci doesn't support CCF yet. So
at the moment all this discussion is a bit moot. I'd propose leaving
the driver as it is for now, because that at least makes it useful on
am33xx. And when CCF and davinci have the needed support in place,
then send a patch to change the clock handling in tilcdc. I don't
actually have any davinci hw to test on, but I can easily test on
am33xx.
BR,
-R
Regards
Afzal
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
Hi Rob,
As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC,
I had a look at your IT LCD driver. Comments below.
On Tue, 22 Jan 2013 16:36:22 -0600
Rob Clark robdcl...@gmail.com wrote:
A simple DRM/KMS driver for the TI LCD Controller found in various
smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the
CMA helpers. Currently only the TFP410 DVI encoder is supported
(tested with beaglebone + DVI cape). There are also various LCD
displays, for which support can be added (as I get hw to test on),
and an external i2c HDMI encoder found on some boards.
The display controller supports a single CRTC. And the encoder+
connector are split out into sub-devices. Depending on which LCD
or external encoder is actually present, the appropriate output
module(s) will be loaded.
v1: original
v2: fix fb refcnting and few other cleanups
v3: get +/- vsync/hsync from timings rather than panel-info, add
option DT max-bandwidth field so driver doesn't attempt to
pick a display mode with too high memory bandwidth, and other
small cleanups
Signed-off-by: Rob Clark robdcl...@gmail.com
---
drivers/gpu/drm/Kconfig| 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/tilcdc/Kconfig | 10 +
drivers/gpu/drm/tilcdc/Makefile| 8 +
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 597
drivers/gpu/drm/tilcdc/tilcdc_drv.c| 605
+
drivers/gpu/drm/tilcdc/tilcdc_drv.h| 159 +
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 154 +
drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++
drivers/gpu/drm/tilcdc/tilcdc_tfp410.h | 26 ++
10 files changed, 1985 insertions(+)
create mode 100644 drivers/gpu/drm/tilcdc/Kconfig
create mode 100644 drivers/gpu/drm/tilcdc/Makefile
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h
[snip]
diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
new file mode 100644
index 000..ee9b592
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/Kconfig
@@ -0,0 +1,10 @@
+config DRM_TILCDC
+ tristate DRM Support for TI LCDC Display Controller
+ depends on DRM OF
+ select DRM_KMS_HELPER
+ select DRM_KMS_CMA_HELPER
+ select DRM_GEM_CMA_HELPER
+ help
+ Choose this option if you have an TI SoC with LCDC display
+ controller, for example AM33xx in beagle-bone, DA8xx, or
+ OMAP-L1xx. This driver replaces the FB_DA8XX fbdev driver.
diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
new file mode 100644
index 000..1359cc2
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -0,0 +1,8 @@
+ccflags-y := -Iinclude/drm -Werror
+
+tilcdc-y := \
+ tilcdc_crtc.o \
+ tilcdc_tfp410.o \
+ tilcdc_drv.o
As I understand, this means that the 3 objects will go into the
resident kernel.
I though that the device tree was created for Linux distributors who
might generate generic ARM kernels, the choice of the drivers being
done according the local device tree.
From this point of vue, it would be nicer to have 2 separate modules:
tilcdc and tfp410, tilcdc_crtc being included in one of these ones
(I did not look deep enough at the code to know which).
[snip]
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
new file mode 100644
index 000..cf1fddc
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -0,0 +1,605 @@
[snip]
+static struct drm_driver tilcdc_driver = {
+ .driver_features= DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET,
+ .load = tilcdc_load,
+ .unload = tilcdc_unload,
+ .preclose = tilcdc_preclose,
+ .lastclose = tilcdc_lastclose,
+ .irq_handler= tilcdc_irq,
+ .irq_preinstall = tilcdc_irq_preinstall,
+ .irq_postinstall= tilcdc_irq_postinstall,
+ .irq_uninstall = tilcdc_irq_uninstall,
+ .get_vblank_counter = drm_vblank_count,
+ .enable_vblank = tilcdc_enable_vblank,
+ .disable_vblank = tilcdc_disable_vblank,
+ .gem_free_object= drm_gem_cma_free_object,
+ .gem_vm_ops = drm_gem_cma_vm_ops,
+ .dumb_create= drm_gem_cma_dumb_create,
+ .dumb_map_offset= drm_gem_cma_dumb_map_offset,
+ .dumb_destroy = drm_gem_cma_dumb_destroy,
+#ifdef CONFIG_DEBUG_FS
+ .debugfs_init = tilcdc_debugfs_init,
+ .debugfs_cleanup= tilcdc_debugfs_cleanup,
+#endif
+ .fops = fops,
+ .name
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Wed, Jan 23, 2013 at 3:42 AM, Jean-Francois Moine moin...@free.fr wrote:
Hi Rob,
As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC,
I had a look at your IT LCD driver. Comments below.
Just fyi, you can re-use the nxp-tda998x part independently of tilcdc
(just in case that wasn't clear).
It may be that we need to add some configuration info struct, if for
example there are some differences in video signal mux on the dove
board, but that shouldn't be a big deal. I figure we can see what is
needed as others start to use it and add as needed.
On Tue, 22 Jan 2013 16:36:22 -0600
Rob Clark robdcl...@gmail.com wrote:
A simple DRM/KMS driver for the TI LCD Controller found in various
smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the
CMA helpers. Currently only the TFP410 DVI encoder is supported
(tested with beaglebone + DVI cape). There are also various LCD
displays, for which support can be added (as I get hw to test on),
and an external i2c HDMI encoder found on some boards.
The display controller supports a single CRTC. And the encoder+
connector are split out into sub-devices. Depending on which LCD
or external encoder is actually present, the appropriate output
module(s) will be loaded.
v1: original
v2: fix fb refcnting and few other cleanups
v3: get +/- vsync/hsync from timings rather than panel-info, add
option DT max-bandwidth field so driver doesn't attempt to
pick a display mode with too high memory bandwidth, and other
small cleanups
Signed-off-by: Rob Clark robdcl...@gmail.com
---
drivers/gpu/drm/Kconfig| 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/tilcdc/Kconfig | 10 +
drivers/gpu/drm/tilcdc/Makefile| 8 +
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 597
drivers/gpu/drm/tilcdc/tilcdc_drv.c| 605
+
drivers/gpu/drm/tilcdc/tilcdc_drv.h| 159 +
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 154 +
drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++
drivers/gpu/drm/tilcdc/tilcdc_tfp410.h | 26 ++
10 files changed, 1985 insertions(+)
create mode 100644 drivers/gpu/drm/tilcdc/Kconfig
create mode 100644 drivers/gpu/drm/tilcdc/Makefile
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h
[snip]
diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
new file mode 100644
index 000..ee9b592
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/Kconfig
@@ -0,0 +1,10 @@
+config DRM_TILCDC
+ tristate DRM Support for TI LCDC Display Controller
+ depends on DRM OF
+ select DRM_KMS_HELPER
+ select DRM_KMS_CMA_HELPER
+ select DRM_GEM_CMA_HELPER
+ help
+ Choose this option if you have an TI SoC with LCDC display
+ controller, for example AM33xx in beagle-bone, DA8xx, or
+ OMAP-L1xx. This driver replaces the FB_DA8XX fbdev driver.
diff --git a/drivers/gpu/drm/tilcdc/Makefile
b/drivers/gpu/drm/tilcdc/Makefile
new file mode 100644
index 000..1359cc2
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -0,0 +1,8 @@
+ccflags-y := -Iinclude/drm -Werror
+
+tilcdc-y := \
+ tilcdc_crtc.o \
+ tilcdc_tfp410.o \
+ tilcdc_drv.o
As I understand, this means that the 3 objects will go into the
resident kernel.
I though that the device tree was created for Linux distributors who
might generate generic ARM kernels, the choice of the drivers being
done according the local device tree.
From this point of vue, it would be nicer to have 2 separate modules:
tilcdc and tfp410, tilcdc_crtc being included in one of these ones
(I did not look deep enough at the code to know which).
drv and crtc are the core driver... arguably the tfp410 part could
be optional. I'd prefer *not* as a separate module, as this implies
some independence of module lifetime, which is not true. I generally
prefer to start simple and add complexity later if someone really
thinks they need it, which is why I avoided adding a bunch of kconfig
options to start with.
[snip]
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
new file mode 100644
index 000..cf1fddc
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -0,0 +1,605 @@
[snip]
+static struct drm_driver tilcdc_driver = {
+ .driver_features= DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET,
+ .load = tilcdc_load,
+ .unload = tilcdc_unload,
+ .preclose = tilcdc_preclose,
+ .lastclose =
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Wed, Jan 23, 2013 at 07:24:33AM -0600, Rob Clark wrote: On Wed, Jan 23, 2013 at 3:42 AM, Jean-Francois Moine moin...@free.fr wrote: Hi Rob, As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC, I had a look at your IT LCD driver. Comments below. Just fyi, you can re-use the nxp-tda998x part independently of tilcdc (just in case that wasn't clear). Great, this chip is also used on the cubox too. The one thing I wonder is how you deal with the VSYNC/HSYNC polarities that are provided to the TDA9988x chip. On the cubox, I have to adjust the mode parameters such that the polarities are fixed up thusly: adjusted-flags = ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_PCSYNC | DRM_MODE_FLAG_NCSYNC); /* The TDA19988 always requires negative VSYNC? */ adjusted-flags |= DRM_MODE_FLAG_NVSYNC; /* The TDA19988 requires positive HSYNC on 1080p or 720p */ if ((adjusted-hdisplay == 1920 adjusted-vdisplay == 1080) || (adjusted-hdisplay == 1280 adjusted-vdisplay == 720)) adjusted-flags |= DRM_MODE_FLAG_PHSYNC; else adjusted-flags |= DRM_MODE_FLAG_NHSYNC; return true; for these resolutions to be displayed correctly. Also, when the only output is the HDMI device, reporting the display disconnected and without any modes seems to cause problems - I have to report unknown status when there's nothing connected, and also provide a default (720p) mode when no EDID is received. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Wed, Jan 23, 2013 at 7:36 AM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Wed, Jan 23, 2013 at 07:24:33AM -0600, Rob Clark wrote: On Wed, Jan 23, 2013 at 3:42 AM, Jean-Francois Moine moin...@free.fr wrote: Hi Rob, As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC, I had a look at your IT LCD driver. Comments below. Just fyi, you can re-use the nxp-tda998x part independently of tilcdc (just in case that wasn't clear). Great, this chip is also used on the cubox too. cool, it would be great if other platforms could benefit from this as well.. the out-of-tree nxp driver is just horrid ;-) The one thing I wonder is how you deal with the VSYNC/HSYNC polarities that are provided to the TDA9988x chip. On the cubox, I have to adjust the mode parameters such that the polarities are fixed up thusly: adjusted-flags = ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_PCSYNC | DRM_MODE_FLAG_NCSYNC); /* The TDA19988 always requires negative VSYNC? */ adjusted-flags |= DRM_MODE_FLAG_NVSYNC; /* The TDA19988 requires positive HSYNC on 1080p or 720p */ if ((adjusted-hdisplay == 1920 adjusted-vdisplay == 1080) || (adjusted-hdisplay == 1280 adjusted-vdisplay == 720)) adjusted-flags |= DRM_MODE_FLAG_PHSYNC; else adjusted-flags |= DRM_MODE_FLAG_NHSYNC; return true; for these resolutions to be displayed correctly. hmm, I didn't come across this. Although 1080p seems to be a bit more than what the little board I'm working on can drive. I didn't have to do any special fixup for 720p.. I wonder if you are having to do that with the nxp out of tree driver? Maybe it is related to how that driver it is configuring the hw? It would be interesting if you hit the same issue w/ the i2c encoder slave version. At any rate, if it turns out to be needed, we can add this in tda998x_encoder_mode_fixup(). Also, when the only output is the HDMI device, reporting the display disconnected and without any modes seems to cause problems - I have to report unknown status when there's nothing connected, and also provide a default (720p) mode when no EDID is received. hmm, also did not run into any issues here on my end. What sort of problems did you hit? BR, -R -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Wed, Jan 23, 2013 at 8:13 AM, Rob Clark robdcl...@gmail.com wrote: On Wed, Jan 23, 2013 at 7:36 AM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Wed, Jan 23, 2013 at 07:24:33AM -0600, Rob Clark wrote: On Wed, Jan 23, 2013 at 3:42 AM, Jean-Francois Moine moin...@free.fr wrote: Hi Rob, As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC, I had a look at your IT LCD driver. Comments below. Just fyi, you can re-use the nxp-tda998x part independently of tilcdc (just in case that wasn't clear). Great, this chip is also used on the cubox too. cool, it would be great if other platforms could benefit from this as well.. the out-of-tree nxp driver is just horrid ;-) The one thing I wonder is how you deal with the VSYNC/HSYNC polarities that are provided to the TDA9988x chip. On the cubox, I have to adjust the mode parameters such that the polarities are fixed up thusly: adjusted-flags = ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_PCSYNC | DRM_MODE_FLAG_NCSYNC); /* The TDA19988 always requires negative VSYNC? */ adjusted-flags |= DRM_MODE_FLAG_NVSYNC; /* The TDA19988 requires positive HSYNC on 1080p or 720p */ if ((adjusted-hdisplay == 1920 adjusted-vdisplay == 1080) || (adjusted-hdisplay == 1280 adjusted-vdisplay == 720)) adjusted-flags |= DRM_MODE_FLAG_PHSYNC; else adjusted-flags |= DRM_MODE_FLAG_NHSYNC; return true; for these resolutions to be displayed correctly. hmm, I didn't come across this. Although 1080p seems to be a bit more than what the little board I'm working on can drive. I didn't have to do any special fixup for 720p.. one thought.. I haven't enabled any hdmi features like audio yet.. which could be a reason that I didn't hit some of these issues. I'm not really much of an audio/asoc expert so I'm not really sure how this should hook in for audio stuff, but if someone out there with some hw with a similar nxp encoder chip does have some better audio experience, I'd be interested to work together on that. BR, -R I wonder if you are having to do that with the nxp out of tree driver? Maybe it is related to how that driver it is configuring the hw? It would be interesting if you hit the same issue w/ the i2c encoder slave version. At any rate, if it turns out to be needed, we can add this in tda998x_encoder_mode_fixup(). Also, when the only output is the HDMI device, reporting the display disconnected and without any modes seems to cause problems - I have to report unknown status when there's nothing connected, and also provide a default (720p) mode when no EDID is received. hmm, also did not run into any issues here on my end. What sort of problems did you hit? BR, -R -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
On Tue, Jan 22, 2013 at 04:36:22PM -0600, Rob Clark wrote:
A simple DRM/KMS driver for the TI LCD Controller found in various
smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the
CMA helpers. Currently only the TFP410 DVI encoder is supported
(tested with beaglebone + DVI cape). There are also various LCD
displays, for which support can be added (as I get hw to test on),
and an external i2c HDMI encoder found on some boards.
The display controller supports a single CRTC. And the encoder+
connector are split out into sub-devices. Depending on which LCD
or external encoder is actually present, the appropriate output
module(s) will be loaded.
v1: original
v2: fix fb refcnting and few other cleanups
v3: get +/- vsync/hsync from timings rather than panel-info, add
option DT max-bandwidth field so driver doesn't attempt to
pick a display mode with too high memory bandwidth, and other
small cleanups
Signed-off-by: Rob Clark robdcl...@gmail.com
Ok, read through it, looks nice. No idea whether this should use the panel
stuff, but since no-one else does who cares. A few nits and questions
below. Was a nice reading to learn about kfifo and the runtime power
stuff.
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/Kconfig| 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/tilcdc/Kconfig | 10 +
drivers/gpu/drm/tilcdc/Makefile| 8 +
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 597
drivers/gpu/drm/tilcdc/tilcdc_drv.c| 605
+
drivers/gpu/drm/tilcdc/tilcdc_drv.h| 159 +
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 154 +
drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++
drivers/gpu/drm/tilcdc/tilcdc_tfp410.h | 26 ++
10 files changed, 1985 insertions(+)
create mode 100644 drivers/gpu/drm/tilcdc/Kconfig
create mode 100644 drivers/gpu/drm/tilcdc/Makefile
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h
[cut]
+struct tilcdc_crtc {
+ struct drm_crtc base;
+
+ const struct tilcdc_panel_info *info;
+ uint32_t dirty;
+ dma_addr_t start, end;
+ struct drm_pending_vblank_event *event;
+ int dpms;
+ wait_queue_head_t frame_done_wq;
+ bool frame_done;
+
+ /* fb currently set to scanout 0/1: */
+ struct drm_framebuffer *scanout[2];
+
+ /* for deferred fb unref's: */
+ DECLARE_KFIFO_PTR(unref_fifo, struct drm_framebuffer *);
+ struct work_struct work;
+};
+#define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
+
+static void unref_worker(struct work_struct *work)
+{
+ struct tilcdc_crtc *tilcdc_crtc = container_of(work, struct
tilcdc_crtc, work);
+ struct drm_device *dev = tilcdc_crtc-base.dev;
+ struct drm_framebuffer *fb;
+
+ mutex_lock(dev-mode_config.mutex);
+ while (kfifo_get(tilcdc_crtc-unref_fifo, fb))
+ drm_framebuffer_unreference(fb);
+ mutex_unlock(dev-mode_config.mutex);
Hm, just learned about the kfifo api. It looks like the locking here still
works even with the new modeset locking, since kfifo explicitly allows
concurrent readers and writers without locking, as long as there's only on
of each kind. But maybe switch over to crtc-mutex to make things less
tricky.
Also, kfifo seems to have a new api which allows embedding of the kfifo
thing and needs to be used with kfifo_in/out.
[cut]
+static void update_scanout(struct drm_crtc *crtc)
+{
+ struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+ struct drm_device *dev = crtc-dev;
+ struct drm_framebuffer *fb = crtc-fb;
+ struct drm_gem_cma_object *gem;
+ unsigned int depth, bpp;
+
+ drm_fb_get_bpp_depth(fb-pixel_format, depth, bpp);
+ gem = drm_fb_cma_get_gem_obj(fb, 0);
+
+ tilcdc_crtc-start = gem-paddr + fb-offsets[0] +
+ (crtc-y * fb-pitches[0]) + (crtc-x * bpp/8);
+
+ tilcdc_crtc-end = tilcdc_crtc-start +
+ (crtc-mode.vdisplay * fb-pitches[0]);
+
+ if (tilcdc_crtc-dpms == DRM_MODE_DPMS_ON) {
+ /* already enabled, so just mark the frames that need
+ * updating and they will be updated on vblank:
+ */
+ tilcdc_crtc-dirty |= LCDC_END_OF_FRAME0 | LCDC_END_OF_FRAME1;
+ drm_vblank_get(dev, 0);
+ } else {
+ /* not enabled yet, so update registers immediately: */
+ set_scanout(crtc, 0);
+ set_scanout(crtc, 1);
At least on intel we disallow pageflips on disabled crtcs since
drm_vblank_get
Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
Op 22 jan. 2013, om 23:36 heeft Rob Clark robdcl...@gmail.com het volgende geschreven: A simple DRM/KMS driver for the TI LCD Controller found in various smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the CMA helpers. Currently only the TFP410 DVI encoder is supported (tested with beaglebone + DVI cape). There are also various LCD displays, for which support can be added (as I get hw to test on), and an external i2c HDMI encoder found on some boards. The display controller supports a single CRTC. And the encoder+ connector are split out into sub-devices. Depending on which LCD or external encoder is actually present, the appropriate output module(s) will be loaded. v1: original v2: fix fb refcnting and few other cleanups v3: get +/- vsync/hsync from timings rather than panel-info, add option DT max-bandwidth field so driver doesn't attempt to pick a display mode with too high memory bandwidth, and other small cleanups Signed-off-by: Rob Clark robdcl...@gmail.com Tested on a beaglebone and beaglebone-black: Tested-by: Koen Kooi k...@dominion.thruhere.net-- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
