This series tries to make interface drivers less dependent on omap_dss_device
which represents a panel/device connected to that interface. The current way of
configuring an interface is to populate the panel's omap_dss_device instance
with parameters common to the panel and the interface, they are either populated
in the board file, or in the panel driver. Panel timings, number of lanes
connected to interface, and pixel format are examples of such parameters, these
are then extracted by the interface driver to configure itself.
This approach has some disadvantages:
- The omap_dss_device contains fields which could be handled independently by
the panel driver. For example, we have an enum field in omap_dss_device to
tell what mode the panel operates in. This information could be handled by the
panel driver itself. But it's a part of omap_dss_device since we need to pass
this down to the interface driver.
- An interface can't exist by itself. That is, it needs a panel to be connected
to it to configure itself. It's not practical to configure an interface
without a panel, but it's theoretically possible, and we may need it if we
expose the interface as an entity to a user of OMAPDSS. It's also useful if we
represent writeback as an interface, writeback isn't connected to a panel.
- There is a lack of clarity in how the interface configures itself. Since the
interface driver extracts info from omap_dss_device, it's unclear from a panel
driver point of view about what information in omap_dss_device the interface
is using and what it's not using.
- There are issues with checking the correctness of the parameters in
omap_dss_device. We currently fill up the omap_dss_device completely, and then
try to enable the interface, this results in catching a wrong parameter at a
much later point, rather than catching it immediately.
The alternative approach is for the interface drivers to expose functions/api to
the panel drivers to configure such parameters. This way, the panel driver can
pass the parameters to the interface itself, rather than filling up
omap_dss_device. This would need the panel driver to keep a copy of the
parameters so that it can use to configure it later. This resolves all the
issues mentioned above, and also gives us a chance to make a generic set of
function ops for interfaces, this can make a panel driver independent of
the underlying platform.
The current series starts of this work by creating a set_timings function for
all interfaces passing omap_video_timings, this prevents dssdev->panel.timings
references. The first few patches are some minor cleanups which are useful for
the patches which come later.
There are some points on which I need suggestions/clarifications:
- How do we make sure that these functions are called by the panel driver at the
right time? For example, when setting timings for DSI video mode, we would
need to call omapdss_dsi_set_timings() before we call
omapdss_dsi_display_enable(), otherwise the copy of timings contained in DSI
driver data would be invalid. Also, what should the behaviour of such a
set_timings operation if the interface is already enabled. It is clear for
DPI and HDMI, but I'm not clear about what to do about other interfaces. Do we
add checks for the state of the interface/panel?
- A specific issue about DSI/RFBI getting the resolution via
device->driver->get_resolution(), does this provide a result based on panel
rotation? Can this somehow be replaced by timings?
- For SDI, the set_timings operation is simplified, instead of disabling and
then enabling the panel with a new set of timings, only the new timings are
configured. This is similar to what is done in DPI. I am not clear if this
will work for SDI or not.
- There is no set_timings() function for RFBI yet, this needs to be though of
and fixed.
The reference tree and branch:
This is based on Tomi's for-florian-merged branch, and has 2 of his patches which
got missed the last merge window.
This hasn't been tested thoroughly with all interfaces yet. I was interested in
getting some comments.
Archit Taneja (17):
OMAPDSS: APPLY: Constify timings argument in dss_mgr_set_timings
OMAPDSS: DPI: Remove omap_dss_device arguments in
dpi_set_dsi_clk/dpi_set_dispc_clk
OMAPDSS: HDMI: Remove omap_dss_device argument from hdmi_compute_pll
OMAPDSS: DPI: Add locking for DPI interface
OMAPDSS: DPI: Maintain our own timings field in driver data
OMAPDSS: DPI displays: Take care of panel timings in the driver
itself
OMAPDSS: Displays: Add locking in generic DPI panel driver
OMAPDSS: DSI: Maintain own copy of timings in driver data
OMAPDSS: HDMI: Use our own omap_video_timings field when setting
interface timings
OMAPDSS: HDMI: Add a get_timing function for HDMI interface
OMAPDSS: HDMI: Add locking for hdmi interface get/set timing
functions
OMAPDSS: SDI: Create a separate function for timing/clock
configurations
OMAPDSS: SDI: Create a function to set timings
OMAPDSS: SDI: Maintain our own timings field in driver data
OMAPDSS: VENC: Split VENC into interface and panel driver
OMAPDSS: VENC: Maintain our own timings field in driver data
OMAPDSS: VENC: Add a get_timing function for VENC interface
drivers/video/omap2/displays/panel-acx565akm.c | 13 +-
drivers/video/omap2/displays/panel-generic-dpi.c | 75 ++++++-
.../omap2/displays/panel-lgphilips-lb035q02.c | 2 +
.../omap2/displays/panel-nec-nl8048hl11-01b.c | 2 +
drivers/video/omap2/displays/panel-picodlp.c | 3 +
.../video/omap2/displays/panel-sharp-ls037v7dw01.c | 2 +
drivers/video/omap2/displays/panel-taal.c | 2 +
drivers/video/omap2/displays/panel-tfp410.c | 5 +-
.../video/omap2/displays/panel-tpo-td043mtea1.c | 6 +-
drivers/video/omap2/dss/Makefile | 2 +-
drivers/video/omap2/dss/apply.c | 4 +-
drivers/video/omap2/dss/dpi.c | 52 +++--
drivers/video/omap2/dss/dsi.c | 27 ++-
drivers/video/omap2/dss/dss.h | 19 +-
drivers/video/omap2/dss/hdmi.c | 60 +++--
drivers/video/omap2/dss/hdmi_panel.c | 12 +-
drivers/video/omap2/dss/sdi.c | 87 +++++---
drivers/video/omap2/dss/venc.c | 233 ++++++--------------
drivers/video/omap2/dss/venc_panel.c | 231 +++++++++++++++++++
include/video/omapdss.h | 8 +-
20 files changed, 579 insertions(+), 266 deletions(-)
create mode 100644 drivers/video/omap2/dss/venc_panel.c
--
1.7.9.5
The function dss_mgr_set_timings is supposed to apply timings passed by an
interface driver. It is not supposed to change the timings. Add const qualifier
to the omap_video_timings pointer argument in dss_mgr_set_timings().
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/apply.c | 4 ++--
drivers/video/omap2/dss/dss.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
The clock related info for DSS modules is not tied anymore to a panel's
omap_dss_device struct. It's now passed via platform data. The functions
dpi_set_dsi_clk() and dpi_set_dispc_clk() do not need the omap_dss_device
argument to retieve these clocks. They use the api dss_get_platform_clock_config
to get the clocks. Remove the omap_dss_device arguments from these functions.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dpi.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
The clock related info for DSS modules is not tied anymore to a panel's
omap_dss_device struct. It's now passed via platform data. The function
hdmi_compute_pll() do not need the omap_dss_device argument to retieve these
clocks. They use the api dss_get_platform_clock_config() get the clocks. Remove
the omap_dss_device argument from hdmi_compute_pll()
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/hdmi.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
The DPI interface driver currently relies on the panel driver to ensure calls
like omapdss_dpi_display_enable() and omapdss_dpi_display_disable() are executed
sequentially. Also, currently, there is no way to protect the DPI driver data.
All DPI panel drivers don't ensure this, and in general, a DPI panel driver
should use it's lock to that ensure it's own driver data and omap_dss_device
states are taken care of, and not worry about the DPI interface.
Add mutex locking in the DPI enable/disable/set_timings ops.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dpi.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
@@ -184,14 +186,18 @@ int omapdss_dpi_display_enable(struct omap_dss_device *dssdev){intr;+mutex_lock(&dpi.lock);+if(cpu_is_omap34xx()&&!dpi.vdds_dsi_reg){DSSERR("no VDSS_DSI regulator\n");-return-ENODEV;+r=-ENODEV;+gotoerr_no_reg;}if(dssdev->manager=NULL){DSSERR("failed to enable display: no manager\n");-return-ENODEV;+r=-ENODEV;+gotoerr_no_mgr;}if(dpi_use_dsi_pll(dssdev)){
@@ -238,6 +244,8 @@ int omapdss_dpi_display_enable(struct omap_dss_device *dssdev)if(r)gotoerr_mgr_enable;+mutex_unlock(&dpi.lock);+return0;err_mgr_enable:
The DPI driver currently relies on the timings in omap_dss_device struct to
configure the DISPC accordingly. This makes the DPI interface driver dependent
on the omap_dss_device struct.
Make the DPI driver data maintain it's own timings field. The panel driver is
expected to call dpi_set_timings()(renamed to omapdss_dpi_set_timings) to set
these timings before the panel is enabled.
In the set_timings() op, we still ensure that the omap_dss_device timings
(dssdev->panel.timings) are configured. This will later be configured only by
the DPI panel drivers.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-generic-dpi.c | 4 +++-
.../omap2/displays/panel-lgphilips-lb035q02.c | 2 ++
.../omap2/displays/panel-nec-nl8048hl11-01b.c | 2 ++
drivers/video/omap2/displays/panel-picodlp.c | 3 +++
.../video/omap2/displays/panel-sharp-ls037v7dw01.c | 2 ++
drivers/video/omap2/displays/panel-tfp410.c | 4 +++-
.../video/omap2/displays/panel-tpo-td043mtea1.c | 4 +++-
drivers/video/omap2/dss/dpi.c | 11 +++++++----
include/video/omapdss.h | 4 ++--
9 files changed, 27 insertions(+), 9 deletions(-)
@@ -138,7 +139,7 @@ static int dpi_set_dispc_clk(unsigned long pck_req, unsigned long *fck,staticintdpi_set_mode(structomap_dss_device*dssdev){-structomap_video_timings*t=&dssdev->panel.timings;+structomap_video_timings*t=&dpi.timings;intlck_div=0,pck_div=0;unsignedlongfck=0;unsignedlongpck;
The timings maintained in omap_dss_device(dssdev->panel.timings) should be
maintained by the panel driver itself. It's the panel drivers responsibility
to update it if a new set of timings is to be configured. The DPI interface
driver shouldn't be responsible of updating the panel timings, it's responsible
of maintianing it's own copy of timings.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-generic-dpi.c | 2 ++
drivers/video/omap2/displays/panel-tfp410.c | 1 +
.../video/omap2/displays/panel-tpo-td043mtea1.c | 2 ++
drivers/video/omap2/dss/dpi.c | 1 -
4 files changed, 5 insertions(+), 1 deletion(-)
The generic DPI panel driver doesn't currently have locking to ensure that
the display states and the driver data is maintained correctly. Add mutex
locking to take care of this. Add a new get_timings driver op to override the
default get_timings op. The new driver op contains locking to ensure the correct
panel timings are seen when a DSS2 user calls device->driver->get_timings.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-generic-dpi.c | 69 +++++++++++++++++++---
1 file changed, 62 insertions(+), 7 deletions(-)
The DSI driver currently relies on the timings in omap_dss_device struct to
configure the DISPC and DSI blocks accordingly. This makes the DSI interface
driver dependent on the omap_dss_device struct.
Make the DPI driver data maintain it's own timings field. The panel driver is
expected to call omapdss_dsi_set_timings() to set these timings before the panel
is enabled.
Signed-off-by: Archit Taneja <redacted>d
---
drivers/video/omap2/displays/panel-taal.c | 2 ++
drivers/video/omap2/dss/dsi.c | 27 ++++++++++++++++++++++-----
include/video/omapdss.h | 2 ++
3 files changed, 26 insertions(+), 5 deletions(-)
The hdmi driver currently updates only the 'code' member of hdmi_config when
the op omapdss_hdmi_display_set_timing() is called by the hdmi panel driver.
The 'timing' field of hdmi_config is updated only when hdmi_power_on is called.
It makes more sense to configure the whole hdmi_config field in the set_timing
op called by the panel driver. This way, we don't need to call both functions
to ensure that our hdmi_config is configured correctly. Also, we don't need to
calculate hdmi_config during hdmi_power_on, or rely on the omap_video_timings
in the panel's omap_dss_device struct.
A default timing is now configured in hdmi's probe if the panel driver doesn't
set any timing, or doesn't set a valid timing before enabling the panel. Also,
when setting manager timings, use the omap_video_timing calculated by
hdmi_get_timings(), this returns the timings as specified in the CEA/VESA
tables, don't use the one provided by the panel driver directly.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dss.h | 3 ++-
drivers/video/omap2/dss/hdmi.c | 41 +++++++++++++++++-----------------
drivers/video/omap2/dss/hdmi_panel.c | 2 +-
3 files changed, 23 insertions(+), 23 deletions(-)
@@ -462,7 +462,6 @@ static int hdmi_power_on(struct omap_dss_device *dssdev){conststructomapdss_clock_config*clks;intr;-conststructhdmi_config*timing;structomap_video_timings*p;unsignedlongphy;
@@ -472,22 +471,10 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)dss_mgr_disable(dssdev->manager);-p=&dssdev->panel.timings;+p=&hdmi.ip_data.cfg.timings;-DSSDBG("hdmi_power_on x_res= %d y_res = %d\n",-dssdev->panel.timings.x_res,-dssdev->panel.timings.y_res);+DSSDBG("hdmi_power_on x_res= %d y_res = %d\n",p->x_res,p->y_res);-timing=hdmi_get_timings();-if(timing=NULL){-/* HDMI code 4 corresponds to 640 * 480 VGA */-hdmi.ip_data.cfg.cm.code=4;-/* DVI mode 1 corresponds to HDMI 0 to DVI */-hdmi.ip_data.cfg.cm.mode=HDMI_DVI;-hdmi.ip_data.cfg=vesa_timings[0];-}else{-hdmi.ip_data.cfg=*timing;-}phy=p->pixel_clock;hdmi_compute_pll(phy,&hdmi.ip_data.pll_data);
@@ -525,7 +512,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)dispc_enable_gamma_table(0);/* tv size */-dss_mgr_set_timings(dssdev->manager,&dssdev->panel.timings);+dss_mgr_set_timings(dssdev->manager,p);r=hdmi.ip_data.ops->video_enable(&hdmi.ip_data);if(r)
@@ -571,13 +558,18 @@ int omapdss_hdmi_display_check_timing(struct omap_dss_device *dssdev,}-voidomapdss_hdmi_display_set_timing(structomap_dss_device*dssdev)+voidomapdss_hdmi_display_set_timing(structomap_dss_device*dssdev,+structomap_video_timings*timings){structhdmi_cmcm;+conststructhdmi_config*timing;++cm=hdmi_get_code(timings);+hdmi.ip_data.cfg.cm=cm;-cm=hdmi_get_code(&dssdev->panel.timings);-hdmi.ip_data.cfg.cm.code=cm.code;-hdmi.ip_data.cfg.cm.mode=cm.mode;+timing=hdmi_get_timings();+if(timing!=NULL)+hdmi.ip_data.cfg=*timing;if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){intr;
@@ -588,7 +580,7 @@ void omapdss_hdmi_display_set_timing(struct omap_dss_device *dssdev)if(r)DSSERR("failed to power on device\n");}else{-dss_mgr_set_timings(dssdev->manager,&dssdev->panel.timings);+dss_mgr_set_timings(dssdev->manager,&timing->timings);}}
@@ -933,6 +925,13 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)hdmi.ip_data.core_av_offset=HDMI_CORE_AV;hdmi.ip_data.pll_offset=HDMI_PLLCTRL;hdmi.ip_data.phy_offset=HDMI_PHY;++/*+*initializehdmitimingstodefaultvalue:+*HDMIcode4(VGA)andHDMImode1(DVI)+*/+hdmi.ip_data.cfg=vesa_timings[0];+mutex_init(&hdmi.ip_data.lock);hdmi_panel_init();
Add function omapdss_hdmi_display_get_timing() which returns the timings
maintained by the HDMI interface driver in it's hdmi_config field. This
prevents the need for the panel driver to configure default timings in it's
probe.
This function is just intended to be used once during the panel driver's probe.
It makes sense for those interfaces which can be configured to a default timing.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dss.h | 2 ++
drivers/video/omap2/dss/hdmi.c | 6 ++++++
drivers/video/omap2/dss/hdmi_panel.c | 10 +++++-----
3 files changed, 13 insertions(+), 5 deletions(-)
The hdmi interface driver exposes functions to the hdmi panel driver to
get and configure the interface timings maintained by the hdmi driver.
These timings(stored in hdmi.ip_data.cfg) should be protected by the hdmi lock
to ensure they are called sequentially, this is similar to how hdmi enable and
disable functions need locking.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/hdmi.c | 8 ++++++++
1 file changed, 8 insertions(+)
Create a function sdi_set_mode() which configures the DISPC and DSS(PRCM)
clocks to get the required pixel clock, and configure the manager timings.
This is similar to what's done in the DPI driver in dpi_set_mode().
This makes the code a bit cleaner to read, and makes it easier to reconfigure
timings instead of switching off the whole interface, and then enabling the
interface with the new timings.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/sdi.c | 65 +++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 28 deletions(-)
@@ -49,40 +49,21 @@ static void sdi_config_lcd_manager(struct omap_dss_device *dssdev)dss_mgr_set_lcd_config(dssdev->manager,&sdi.mgr_config);}-intomapdss_sdi_display_enable(structomap_dss_device*dssdev)+staticintsdi_set_mode(structomap_dss_device*dssdev){+intr;structomap_video_timings*t=&dssdev->panel.timings;structdss_clock_infodss_cinfo;structdispc_clock_infodispc_cinfo;unsignedlongpck;-intr;--if(dssdev->manager=NULL){-DSSERR("failed to enable display: no manager\n");-return-ENODEV;-}--r=omap_dss_start_device(dssdev);-if(r){-DSSERR("failed to start device\n");-gotoerr_start_dev;-}--r=regulator_enable(sdi.vdds_sdi_reg);-if(r)-gotoerr_reg_enable;--r=dispc_runtime_get();-if(r)-gotoerr_get_dispc;/* 15.5.9.1.2 */-dssdev->panel.timings.data_pclk_edge=OMAPDSS_DRIVE_SIG_RISING_EDGE;-dssdev->panel.timings.sync_pclk_edge=OMAPDSS_DRIVE_SIG_RISING_EDGE;+t->data_pclk_edge=OMAPDSS_DRIVE_SIG_RISING_EDGE;+t->sync_pclk_edge=OMAPDSS_DRIVE_SIG_RISING_EDGE;r=dss_calc_clock_div(t->pixel_clock*1000,&dss_cinfo,&dispc_cinfo);if(r)-gotoerr_calc_clock_div;+returnr;sdi.mgr_config.clock_info=dispc_cinfo;
@@ -96,12 +77,41 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)t->pixel_clock=pck;}-dss_mgr_set_timings(dssdev->manager,t);r=dss_set_clock_div(&dss_cinfo);if(r)-gotoerr_set_dss_clock_div;+returnr;++return0;+}++intomapdss_sdi_display_enable(structomap_dss_device*dssdev)+{+intr;++if(dssdev->manager=NULL){+DSSERR("failed to enable display: no manager\n");+return-ENODEV;+}++r=omap_dss_start_device(dssdev);+if(r){+DSSERR("failed to start device\n");+gotoerr_start_dev;+}++r=regulator_enable(sdi.vdds_sdi_reg);+if(r)+gotoerr_reg_enable;++r=dispc_runtime_get();+if(r)+gotoerr_get_dispc;++r=sdi_set_mode(dssdev);+if(r)+gotoerr_set_mode;sdi_config_lcd_manager(dssdev);
@@ -120,8 +130,7 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)err_mgr_enable:dss_sdi_disable();err_sdi_enable:-err_set_dss_clock_div:-err_calc_clock_div:+err_set_mode:dispc_runtime_put();err_get_dispc:regulator_disable(sdi.vdds_sdi_reg);
Create function omapdss_sdi_set_timings(), this can be used by a SDI panel
driver without disabling/enabling the SDI interface. This is similar to the
set_timings op of the DPI interface driver. It calls sdi_set_mode() which only
configures the DISPC timings and DSS/DISPC clock dividers.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-acx565akm.c | 13 +------------
drivers/video/omap2/dss/sdi.c | 19 +++++++++++++++++++
include/video/omapdss.h | 2 ++
3 files changed, 22 insertions(+), 12 deletions(-)
The SDI driver currently relies on the timings in omap_dss_device struct to
configure the DISPC accordingly. This makes the SDI interface driver dependent
on the omap_dss_device struct.
Make the SDI driver data maintain it's own timings field. The panel driver is
expected to call omapdss_sdi_set_timings() to set these timings before the panel
is enabled.
Make the SDI panel driver configure the new timings is the omap_dss_device
struct(dssdev->panel.timings). The SDI driver is responsible for maintaining
only it's own copy of timings.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-acx565akm.c | 4 ++++
drivers/video/omap2/dss/sdi.c | 5 +++--
2 files changed, 7 insertions(+), 2 deletions(-)
The current venc.c driver contains both the interface and panel driver code.
This makes the driver hard to read, and difficult to understand the work split
between the interface and panel driver and the how the locking works.
This also makes it easier to clearly define the VENC interface ops called by the
panel driver.
Split venc.c into venc.c and venc_panel.c representing the interface and panel
driver respectively. This split is done along the lines of the HDMI interface
and panel drivers.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/Makefile | 2 +-
drivers/video/omap2/dss/dss.h | 10 ++
drivers/video/omap2/dss/venc.c | 208 ++++++++----------------------
drivers/video/omap2/dss/venc_panel.c | 231 ++++++++++++++++++++++++++++++++++
4 files changed, 295 insertions(+), 156 deletions(-)
create mode 100644 drivers/video/omap2/dss/venc_panel.c
@@ -491,171 +490,95 @@ unsigned long venc_get_pixel_clock(void)return13500000;}-staticssize_tdisplay_output_type_show(structdevice*dev,-structdevice_attribute*attr,char*buf)+intomapdss_venc_display_enable(structomap_dss_device*dssdev){-structomap_dss_device*dssdev=to_dss_device(dev);-constchar*ret;--switch(dssdev->phy.venc.type){-caseOMAP_DSS_VENC_TYPE_COMPOSITE:-ret="composite";-break;-caseOMAP_DSS_VENC_TYPE_SVIDEO:-ret="svideo";-break;-default:-return-EINVAL;-}+intr;-returnsnprintf(buf,PAGE_SIZE,"%s\n",ret);-}--staticssize_tdisplay_output_type_store(structdevice*dev,-structdevice_attribute*attr,constchar*buf,size_tsize)-{-structomap_dss_device*dssdev=to_dss_device(dev);-enumomap_dss_venc_typenew_type;--if(sysfs_streq("composite",buf))-new_type=OMAP_DSS_VENC_TYPE_COMPOSITE;-elseif(sysfs_streq("svideo",buf))-new_type=OMAP_DSS_VENC_TYPE_SVIDEO;-else-return-EINVAL;+DSSDBG("venc_display_enable\n");mutex_lock(&venc.venc_lock);-if(dssdev->phy.venc.type!=new_type){-dssdev->phy.venc.type=new_type;-if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){-venc_power_off(dssdev);-venc_power_on(dssdev);-}+if(dssdev->manager=NULL){+DSSERR("Failed to enable display: no manager\n");+r=-ENODEV;+gotoerr0;}-mutex_unlock(&venc.venc_lock);--returnsize;-}--staticDEVICE_ATTR(output_type,S_IRUGO|S_IWUSR,-display_output_type_show,display_output_type_store);--/* driver */-staticintvenc_panel_probe(structomap_dss_device*dssdev)-{-dssdev->panel.timings=omap_dss_pal_timings;--returndevice_create_file(&dssdev->dev,&dev_attr_output_type);-}--staticvoidvenc_panel_remove(structomap_dss_device*dssdev)-{-device_remove_file(&dssdev->dev,&dev_attr_output_type);-}--staticintvenc_panel_enable(structomap_dss_device*dssdev)-{-intr=0;--DSSDBG("venc_enable_display\n");--mutex_lock(&venc.venc_lock);-r=omap_dss_start_device(dssdev);if(r){DSSERR("failed to start device\n");gotoerr0;}-if(dssdev->state!=OMAP_DSS_DISPLAY_DISABLED){-r=-EINVAL;-gotoerr1;-}+if(dssdev->platform_enable)+dssdev->platform_enable(dssdev);-r=venc_runtime_get();-if(r)-gotoerr1;r=venc_power_on(dssdev);if(r)-gotoerr2;+gotoerr1;venc.wss_data=0;-dssdev->state=OMAP_DSS_DISPLAY_ACTIVE;-mutex_unlock(&venc.venc_lock);+return0;-err2:-venc_runtime_put();err1:+if(dssdev->platform_disable)+dssdev->platform_disable(dssdev);omap_dss_stop_device(dssdev);err0:mutex_unlock(&venc.venc_lock);-returnr;}-staticvoidvenc_panel_disable(structomap_dss_device*dssdev)+voidomapdss_venc_display_disable(structomap_dss_device*dssdev){-DSSDBG("venc_disable_display\n");+DSSDBG("venc_display_disable\n");mutex_lock(&venc.venc_lock);-if(dssdev->state=OMAP_DSS_DISPLAY_DISABLED)-gotoend;--if(dssdev->state=OMAP_DSS_DISPLAY_SUSPENDED){-/* suspended is the same as disabled with venc */-dssdev->state=OMAP_DSS_DISPLAY_DISABLED;-gotoend;-}-venc_power_off(dssdev);-venc_runtime_put();--dssdev->state=OMAP_DSS_DISPLAY_DISABLED;-omap_dss_stop_device(dssdev);-end:-mutex_unlock(&venc.venc_lock);-}-staticintvenc_panel_suspend(structomap_dss_device*dssdev)-{-venc_panel_disable(dssdev);-return0;-}+if(dssdev->platform_disable)+dssdev->platform_disable(dssdev);-staticintvenc_panel_resume(structomap_dss_device*dssdev)-{-returnvenc_panel_enable(dssdev);+mutex_unlock(&venc.venc_lock);}-staticvoidvenc_set_timings(structomap_dss_device*dssdev,-structomap_video_timings*timings)+voidomapdss_venc_set_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings){DSSDBG("venc_set_timings\n");+mutex_lock(&venc.venc_lock);+/* Reset WSS data when the TV standard changes. */if(memcmp(&dssdev->panel.timings,timings,sizeof(*timings)))venc.wss_data=0;dssdev->panel.timings=*timings;+if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){+intr;+/* turn the venc off and on to get new timings to use */-venc_panel_disable(dssdev);-venc_panel_enable(dssdev);+venc_power_off(dssdev);++r=venc_power_on(dssdev);+if(r)+DSSERR("failed to power on VENC\n");}else{dss_mgr_set_timings(dssdev->manager,timings);}++mutex_unlock(&venc.venc_lock);}-staticintvenc_check_timings(structomap_dss_device*dssdev,-structomap_video_timings*timings)+intomapdss_venc_check_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings){DSSDBG("venc_check_timings\n");
@@ -668,13 +591,13 @@ static int venc_check_timings(struct omap_dss_device *dssdev,return-EINVAL;}-staticu32venc_get_wss(structomap_dss_device*dssdev)+u32omapdss_venc_get_wss(structomap_dss_device*dssdev){/* Invert due to VENC_L21_WC_CTL:INV=1 */return(venc.wss_data>>8)^0xfffff;}-staticintvenc_set_wss(structomap_dss_device*dssdev,u32wss)+intomapdss_venc_set_wss(structomap_dss_device*dssdev,u32wss){conststructvenc_config*config;intr;
@@ -703,31 +626,6 @@ err:returnr;}-staticstructomap_dss_drivervenc_driver={-.probe=venc_panel_probe,-.remove=venc_panel_remove,--.enable=venc_panel_enable,-.disable=venc_panel_disable,-.suspend=venc_panel_suspend,-.resume=venc_panel_resume,--.get_resolution=omapdss_default_get_resolution,-.get_recommended_bpp=omapdss_default_get_recommended_bpp,--.set_timings=venc_set_timings,-.check_timings=venc_check_timings,--.get_wss=venc_get_wss,-.set_wss=venc_set_wss,--.driver={-.name="venc",-.owner=THIS_MODULE,-},-};-/* driver end */-staticint__initvenc_init_display(structomap_dss_device*dssdev){DSSDBG("init_display\n");
@@ -897,9 +795,9 @@ static int __init omap_venchw_probe(struct platform_device *pdev)venc_runtime_put();-r=omap_dss_register_driver(&venc_driver);+r=venc_panel_init();if(r)-gotoerr_reg_panel_driver;+gotoerr_panel_init;dss_debugfs_create_file("venc",venc_dump_regs);
@@ -907,7 +805,7 @@ static int __init omap_venchw_probe(struct platform_device *pdev)return0;-err_reg_panel_driver:+err_panel_init:err_runtime_get:pm_runtime_disable(&pdev->dev);venc_put_clocks();
@@ -923,7 +821,7 @@ static int __exit omap_venchw_remove(struct platform_device *pdev)venc.vdda_dac_reg=NULL;}-omap_dss_unregister_driver(&venc_driver);+venc_panel_exit();pm_runtime_disable(&pdev->dev);venc_put_clocks();
@@ -0,0 +1,231 @@+/*+*Copyright(C)2009NokiaCorporation+*Author:TomiValkeinen<tomi.valkeinen@nokia.com>+*+*VENCpaneldriver+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsoftheGNUGeneralPublicLicenseversion2aspublishedby+*theFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++#include<linux/kernel.h>+#include<linux/err.h>+#include<linux/io.h>+#include<linux/mutex.h>+#include<linux/module.h>++#include<video/omapdss.h>++#include"dss.h"++staticstruct{+structmutexlock;+}venc_panel;++staticssize_tdisplay_output_type_show(structdevice*dev,+structdevice_attribute*attr,char*buf)+{+structomap_dss_device*dssdev=to_dss_device(dev);+constchar*ret;++switch(dssdev->phy.venc.type){+caseOMAP_DSS_VENC_TYPE_COMPOSITE:+ret="composite";+break;+caseOMAP_DSS_VENC_TYPE_SVIDEO:+ret="svideo";+break;+default:+return-EINVAL;+}++returnsnprintf(buf,PAGE_SIZE,"%s\n",ret);+}++staticssize_tdisplay_output_type_store(structdevice*dev,+structdevice_attribute*attr,constchar*buf,size_tsize)+{+structomap_dss_device*dssdev=to_dss_device(dev);+enumomap_dss_venc_typenew_type;++if(sysfs_streq("composite",buf))+new_type=OMAP_DSS_VENC_TYPE_COMPOSITE;+elseif(sysfs_streq("svideo",buf))+new_type=OMAP_DSS_VENC_TYPE_SVIDEO;+else+return-EINVAL;++mutex_lock(&venc_panel.lock);++if(dssdev->phy.venc.type!=new_type){+dssdev->phy.venc.type=new_type;+if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){+omapdss_venc_display_disable(dssdev);+omapdss_venc_display_enable(dssdev);+}+}++mutex_unlock(&venc_panel.lock);++returnsize;+}++staticDEVICE_ATTR(output_type,S_IRUGO|S_IWUSR,+display_output_type_show,display_output_type_store);++staticintvenc_panel_probe(structomap_dss_device*dssdev)+{+mutex_init(&venc_panel.lock);++/* set initial timings to PAL */+dssdev->panel.timings=(structomap_video_timings)+{720,574,13500,64,12,68,5,5,41,+OMAPDSS_SIG_ACTIVE_HIGH,OMAPDSS_SIG_ACTIVE_HIGH,+true,+};++returndevice_create_file(&dssdev->dev,&dev_attr_output_type);+}++staticvoidvenc_panel_remove(structomap_dss_device*dssdev)+{+device_remove_file(&dssdev->dev,&dev_attr_output_type);+}++staticintvenc_panel_enable(structomap_dss_device*dssdev)+{+intr;++dev_dbg(&dssdev->dev,"venc_panel_enable\n");++mutex_lock(&venc_panel.lock);++if(dssdev->state!=OMAP_DSS_DISPLAY_DISABLED){+r=-EINVAL;+gotoerr;+}++r=omapdss_venc_display_enable(dssdev);+if(r)+gotoerr;++dssdev->state=OMAP_DSS_DISPLAY_ACTIVE;++mutex_unlock(&venc_panel.lock);++return0;+err:+mutex_unlock(&venc_panel.lock);++returnr;+}++staticvoidvenc_panel_disable(structomap_dss_device*dssdev)+{+dev_dbg(&dssdev->dev,"venc_panel_disable\n");++mutex_lock(&venc_panel.lock);++if(dssdev->state=OMAP_DSS_DISPLAY_DISABLED)+gotoend;++if(dssdev->state=OMAP_DSS_DISPLAY_SUSPENDED){+/* suspended is the same as disabled with venc */+dssdev->state=OMAP_DSS_DISPLAY_DISABLED;+gotoend;+}++omapdss_venc_display_disable(dssdev);++dssdev->state=OMAP_DSS_DISPLAY_DISABLED;+end:+mutex_unlock(&venc_panel.lock);+}++staticintvenc_panel_suspend(structomap_dss_device*dssdev)+{+venc_panel_disable(dssdev);+return0;+}++staticintvenc_panel_resume(structomap_dss_device*dssdev)+{+returnvenc_panel_enable(dssdev);+}++staticvoidvenc_panel_set_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings)+{+dev_dbg(&dssdev->dev,"venc_panel_set_timings\n");++mutex_lock(&venc_panel.lock);++omapdss_venc_set_timings(dssdev,timings);++mutex_unlock(&venc_panel.lock);+}++staticintvenc_panel_check_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings)+{+dev_dbg(&dssdev->dev,"venc_panel_check_timings\n");++returnomapdss_venc_check_timings(dssdev,timings);+}++staticu32venc_panel_get_wss(structomap_dss_device*dssdev)+{+dev_dbg(&dssdev->dev,"venc_panel_get_wss\n");++returnomapdss_venc_get_wss(dssdev);+}++staticintvenc_panel_set_wss(structomap_dss_device*dssdev,u32wss)+{+dev_dbg(&dssdev->dev,"venc_panel_set_wss\n");++returnomapdss_venc_set_wss(dssdev,wss);+}++staticstructomap_dss_drivervenc_driver={+.probe=venc_panel_probe,+.remove=venc_panel_remove,++.enable=venc_panel_enable,+.disable=venc_panel_disable,+.suspend=venc_panel_suspend,+.resume=venc_panel_resume,++.get_resolution=omapdss_default_get_resolution,+.get_recommended_bpp=omapdss_default_get_recommended_bpp,++.set_timings=venc_panel_set_timings,+.check_timings=venc_panel_check_timings,++.get_wss=venc_panel_get_wss,+.set_wss=venc_panel_set_wss,++.driver={+.name="venc",+.owner=THIS_MODULE,+},+};++intvenc_panel_init(void)+{+returnomap_dss_register_driver(&venc_driver);+}++voidvenc_panel_exit(void)+{+omap_dss_unregister_driver(&venc_driver);+}
The VENC driver currently relies on the timings in omap_dss_device struct to
configure the DISPC and VENC blocks accordingly. This makes the VENC interface
driver dependent on the omap_dss_device struct.
Make the VENC driver data maintain it's own timings field. The panel driver is
expected to call omapdss_venc_set_timings() to set these timings before the
panel is enabled.
Make the VENC panel driver configure the new timings is the omap_dss_device
struct(dssdev->panel.timings). The VENC driver is responsible for maintaining
only it's own copy of timings.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/venc.c | 12 +++++++-----
drivers/video/omap2/dss/venc_panel.c | 1 +
2 files changed, 8 insertions(+), 5 deletions(-)
@@ -432,7 +434,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)gotoerr0;venc_reset();-venc_write_config(venc_timings_to_config(&dssdev->panel.timings));+venc_write_config(venc_timings_to_config(&venc.timings));dss_set_venc_output(dssdev->phy.venc.type);dss_set_dac_pwrdn_bgz(1);
@@ -449,7 +451,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)venc_write_reg(VENC_OUTPUT_CONTROL,l);-dss_mgr_set_timings(dssdev->manager,&dssdev->panel.timings);+dss_mgr_set_timings(dssdev->manager,&venc.timings);r=regulator_enable(venc.vdda_dac_reg);if(r)
@@ -556,10 +558,10 @@ void omapdss_venc_set_timings(struct omap_dss_device *dssdev,mutex_lock(&venc.venc_lock);/* Reset WSS data when the TV standard changes. */-if(memcmp(&dssdev->panel.timings,timings,sizeof(*timings)))+if(memcmp(&venc.timings,timings,sizeof(*timings)))venc.wss_data=0;-dssdev->panel.timings=*timings;+venc.timings=*timings;if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){intr;
@@ -606,7 +608,7 @@ int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss)mutex_lock(&venc.venc_lock);-config=venc_timings_to_config(&dssdev->panel.timings);+config=venc_timings_to_config(&venc.timings);/* Invert due to VENC_L21_WC_CTL:INV=1 */venc.wss_data=(wss^0xfffff)<<8;
Add function omapdss_venc_get_timing() which returns the timings
maintained by the VENC interface driver in it's driver data. This is just used
once by the driver during it's probe. This prevents the need for the panel
driver to configure default timings in it's probe.
Int the VENC interface's probe, the timings field is set to PAL as a default
value. The get_timing op makes more sense for interfaces which can be configured
to a default timing.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dss.h | 2 ++
drivers/video/omap2/dss/venc.c | 13 +++++++++++++
drivers/video/omap2/dss/venc_panel.c | 11 +++++------
3 files changed, 20 insertions(+), 6 deletions(-)
@@ -84,14 +84,13 @@ static DEVICE_ATTR(output_type, S_IRUGO | S_IWUSR,staticintvenc_panel_probe(structomap_dss_device*dssdev){+structomap_video_timingstimings;+mutex_init(&venc_panel.lock);-/* set initial timings to PAL */-dssdev->panel.timings=(structomap_video_timings)-{720,574,13500,64,12,68,5,5,41,-OMAPDSS_SIG_ACTIVE_HIGH,OMAPDSS_SIG_ACTIVE_HIGH,-true,-};+omapdss_venc_get_timings(dssdev,&timings);++dssdev->panel.timings=timings;returndevice_create_file(&dssdev->dev,&dev_attr_output_type);}
On Wednesday 01 August 2012 04:01 PM, Archit Taneja wrote:
This series tries to make interface drivers less dependent on omap_dss_device
which represents a panel/device connected to that interface. The current way of
configuring an interface is to populate the panel's omap_dss_device instance
with parameters common to the panel and the interface, they are either populated
in the board file, or in the panel driver. Panel timings, number of lanes
connected to interface, and pixel format are examples of such parameters, these
are then extracted by the interface driver to configure itself.
This approach has some disadvantages:
- The omap_dss_device contains fields which could be handled independently by
the panel driver. For example, we have an enum field in omap_dss_device to
tell what mode the panel operates in. This information could be handled by the
panel driver itself. But it's a part of omap_dss_device since we need to pass
this down to the interface driver.
- An interface can't exist by itself. That is, it needs a panel to be connected
to it to configure itself. It's not practical to configure an interface
without a panel, but it's theoretically possible, and we may need it if we
expose the interface as an entity to a user of OMAPDSS. It's also useful if we
represent writeback as an interface, writeback isn't connected to a panel.
- There is a lack of clarity in how the interface configures itself. Since the
interface driver extracts info from omap_dss_device, it's unclear from a panel
driver point of view about what information in omap_dss_device the interface
is using and what it's not using.
- There are issues with checking the correctness of the parameters in
omap_dss_device. We currently fill up the omap_dss_device completely, and then
try to enable the interface, this results in catching a wrong parameter at a
much later point, rather than catching it immediately.
The alternative approach is for the interface drivers to expose functions/api to
the panel drivers to configure such parameters. This way, the panel driver can
pass the parameters to the interface itself, rather than filling up
omap_dss_device. This would need the panel driver to keep a copy of the
parameters so that it can use to configure it later. This resolves all the
issues mentioned above, and also gives us a chance to make a generic set of
function ops for interfaces, this can make a panel driver independent of
the underlying platform.
The current series starts of this work by creating a set_timings function for
all interfaces passing omap_video_timings, this prevents dssdev->panel.timings
references. The first few patches are some minor cleanups which are useful for
the patches which come later.
There are some points on which I need suggestions/clarifications:
- How do we make sure that these functions are called by the panel driver at the
right time? For example, when setting timings for DSI video mode, we would
need to call omapdss_dsi_set_timings() before we call
omapdss_dsi_display_enable(), otherwise the copy of timings contained in DSI
driver data would be invalid. Also, what should the behaviour of such a
set_timings operation if the interface is already enabled. It is clear for
DPI and HDMI, but I'm not clear about what to do about other interfaces. Do we
add checks for the state of the interface/panel?
- A specific issue about DSI/RFBI getting the resolution via
device->driver->get_resolution(), does this provide a result based on panel
rotation? Can this somehow be replaced by timings?
- For SDI, the set_timings operation is simplified, instead of disabling and
then enabling the panel with a new set of timings, only the new timings are
configured. This is similar to what is done in DPI. I am not clear if this
will work for SDI or not.
- There is no set_timings() function for RFBI yet, this needs to be though of
and fixed.
The reference tree and branch:
forgot to add the link here:
git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git
pass_timings_interface
Archit
This is based on Tomi's for-florian-merged branch, and has 2 of his patches which
got missed the last merge window.
This hasn't been tested thoroughly with all interfaces yet. I was interested in
getting some comments.
Archit Taneja (17):
OMAPDSS: APPLY: Constify timings argument in dss_mgr_set_timings
OMAPDSS: DPI: Remove omap_dss_device arguments in
dpi_set_dsi_clk/dpi_set_dispc_clk
OMAPDSS: HDMI: Remove omap_dss_device argument from hdmi_compute_pll
OMAPDSS: DPI: Add locking for DPI interface
OMAPDSS: DPI: Maintain our own timings field in driver data
OMAPDSS: DPI displays: Take care of panel timings in the driver
itself
OMAPDSS: Displays: Add locking in generic DPI panel driver
OMAPDSS: DSI: Maintain own copy of timings in driver data
OMAPDSS: HDMI: Use our own omap_video_timings field when setting
interface timings
OMAPDSS: HDMI: Add a get_timing function for HDMI interface
OMAPDSS: HDMI: Add locking for hdmi interface get/set timing
functions
OMAPDSS: SDI: Create a separate function for timing/clock
configurations
OMAPDSS: SDI: Create a function to set timings
OMAPDSS: SDI: Maintain our own timings field in driver data
OMAPDSS: VENC: Split VENC into interface and panel driver
OMAPDSS: VENC: Maintain our own timings field in driver data
OMAPDSS: VENC: Add a get_timing function for VENC interface
drivers/video/omap2/displays/panel-acx565akm.c | 13 +-
drivers/video/omap2/displays/panel-generic-dpi.c | 75 ++++++-
.../omap2/displays/panel-lgphilips-lb035q02.c | 2 +
.../omap2/displays/panel-nec-nl8048hl11-01b.c | 2 +
drivers/video/omap2/displays/panel-picodlp.c | 3 +
.../video/omap2/displays/panel-sharp-ls037v7dw01.c | 2 +
drivers/video/omap2/displays/panel-taal.c | 2 +
drivers/video/omap2/displays/panel-tfp410.c | 5 +-
.../video/omap2/displays/panel-tpo-td043mtea1.c | 6 +-
drivers/video/omap2/dss/Makefile | 2 +-
drivers/video/omap2/dss/apply.c | 4 +-
drivers/video/omap2/dss/dpi.c | 52 +++--
drivers/video/omap2/dss/dsi.c | 27 ++-
drivers/video/omap2/dss/dss.h | 19 +-
drivers/video/omap2/dss/hdmi.c | 60 +++--
drivers/video/omap2/dss/hdmi_panel.c | 12 +-
drivers/video/omap2/dss/sdi.c | 87 +++++---
drivers/video/omap2/dss/venc.c | 233 ++++++--------------
drivers/video/omap2/dss/venc_panel.c | 231 +++++++++++++++++++
include/video/omapdss.h | 8 +-
20 files changed, 579 insertions(+), 266 deletions(-)
create mode 100644 drivers/video/omap2/dss/venc_panel.c
From: Tomi Valkeinen <hidden> Date: 2012-08-07 14:07:42
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
The DSI driver currently relies on the timings in omap_dss_device struct to
configure the DISPC and DSI blocks accordingly. This makes the DSI interface
driver dependent on the omap_dss_device struct.
Make the DPI driver data maintain it's own timings field. The panel driver is
^^^
DSI
quoted hunk
expected to call omapdss_dsi_set_timings() to set these timings before the panel
is enabled.
Signed-off-by: Archit Taneja <redacted>d
---
drivers/video/omap2/displays/panel-taal.c | 2 ++
drivers/video/omap2/dss/dsi.c | 27 ++++++++++++++++++++++-----
include/video/omapdss.h | 2 ++
3 files changed, 26 insertions(+), 5 deletions(-)
@@ -1060,6 +1060,8 @@ static int taal_power_on(struct omap_dss_device *dssdev)gotoerr0;};+omapdss_dsi_set_timings(dssdev,&td->panel_config->timings);+r=omapdss_dsi_display_enable(dssdev);if(r){dev_err(&dssdev->dev,"failed to enable DSI\n");
Video timings for command mode panel are meaningless. If we need to pass
the resolution of the panel, perhaps we should have a separate function
for that.
However, with a quick glance at dsi.c, we don't even use the
dssdev->panel.timings for cmd mode panel. But we do use
dssdev->get_resolution() in a few places. Those calls could be replaced
by storing the panel size in dsi.c, given with omapdss_dsi_set_size() or
such. We could use the timings field in dsi.c to store them, though.
Tomi
From: Tomi Valkeinen <hidden> Date: 2012-08-07 14:20:18
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
Create function omapdss_sdi_set_timings(), this can be used by a SDI panel
driver without disabling/enabling the SDI interface. This is similar to the
set_timings op of the DPI interface driver. It calls sdi_set_mode() which only
configures the DISPC timings and DSS/DISPC clock dividers.
I don't think this works, as the SDI PLL uses pclk-free, and if pclk
changes, PLL lock probably breaks.
OMAP3430 TRM explains the sequence how to configure settings on the fly,
but that's not very simple. Just turning the output off and on is much
easier.
Tomi
From: Tomi Valkeinen <hidden> Date: 2012-08-07 14:32:28
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
This series tries to make interface drivers less dependent on omap_dss_device
which represents a panel/device connected to that interface. The current way of
configuring an interface is to populate the panel's omap_dss_device instance
with parameters common to the panel and the interface, they are either populated
in the board file, or in the panel driver. Panel timings, number of lanes
connected to interface, and pixel format are examples of such parameters, these
are then extracted by the interface driver to configure itself.
The series looks good. I had only a few comments to make, but obviously
this needs quite a bit of testing. I'll try it out.
Tomi
On Tuesday 07 August 2012 07:37 PM, Tomi Valkeinen wrote:
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
quoted
The DSI driver currently relies on the timings in omap_dss_device struct to
configure the DISPC and DSI blocks accordingly. This makes the DSI interface
driver dependent on the omap_dss_device struct.
Make the DPI driver data maintain it's own timings field. The panel driver is
^^^
DSI
quoted
expected to call omapdss_dsi_set_timings() to set these timings before the panel
is enabled.
Signed-off-by: Archit Taneja <redacted>d
---
drivers/video/omap2/displays/panel-taal.c | 2 ++
drivers/video/omap2/dss/dsi.c | 27 ++++++++++++++++++++++-----
include/video/omapdss.h | 2 ++
3 files changed, 26 insertions(+), 5 deletions(-)
@@ -1060,6 +1060,8 @@ static int taal_power_on(struct omap_dss_device *dssdev)gotoerr0;};+omapdss_dsi_set_timings(dssdev,&td->panel_config->timings);+r=omapdss_dsi_display_enable(dssdev);if(r){dev_err(&dssdev->dev,"failed to enable DSI\n");
Video timings for command mode panel are meaningless. If we need to pass
the resolution of the panel, perhaps we should have a separate function
for that.
However, with a quick glance at dsi.c, we don't even use the
dssdev->panel.timings for cmd mode panel. But we do use
dssdev->get_resolution() in a few places. Those calls could be replaced
by storing the panel size in dsi.c, given with omapdss_dsi_set_size() or
such. We could use the timings field in dsi.c to store them, though.
I am a bit unclear about resolution when it comes to command mode panels.
For command mode panels, we can perform rotation at the panel side. That
is, the panel refreshes itself by fetching pixels from it's buffer in a
rotated way. Is that right?
If the original resolution is 864x480, and we set rotation at panel side
to make the rotation 480x864, the DISPC manager size should also be
configured at 480x864 right?
We seem to be setting the manager timings only once when DSI is enabled.
After that, setting rotation doesn't impact manager size.
I am asking this to understand if we need to keep resolution as a
separate parameter than timings. That is, timings represents the initial
width and height of the panel, and resolution represents the current
width and height of the panel.
Archit
On Tuesday 07 August 2012 07:50 PM, Tomi Valkeinen wrote:
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
quoted
Create function omapdss_sdi_set_timings(), this can be used by a SDI panel
driver without disabling/enabling the SDI interface. This is similar to the
set_timings op of the DPI interface driver. It calls sdi_set_mode() which only
configures the DISPC timings and DSS/DISPC clock dividers.
I don't think this works, as the SDI PLL uses pclk-free, and if pclk
changes, PLL lock probably breaks.
OMAP3430 TRM explains the sequence how to configure settings on the fly,
but that's not very simple. Just turning the output off and on is much
easier.
Right, I'll make set_timings() just disable and enable SDI like before.
Archit
From: Tomi Valkeinen <hidden> Date: 2012-08-08 06:15:08
On Wed, 2012-08-08 at 11:27 +0530, Archit Taneja wrote:
I am a bit unclear about resolution when it comes to command mode panels.
Right, it's a bit confusing. And I'm not 100% sure how to manage the
rotation.
For command mode panels, we can perform rotation at the panel side. That
is, the panel refreshes itself by fetching pixels from it's buffer in a
rotated way. Is that right?
Yes. Well, actually I think the panel stores the pixels in rotated
manner when it receives them from OMAP, but it's practically the same.
One thing to realize is that this kind of rotation is a bit limited:
because there's only one buffer, OMAP will write pixels to the buffers
at the same time as the panel shows them. When rotating, this leads to
tearing.
If the panel has double buffer, that solves the problem, but I haven't
seen such panels. Another option is to update the panel in two parts,
like N9 does, but that's timing sensitive and a bit tricky.
If the original resolution is 864x480, and we set rotation at panel side
to make the rotation 480x864, the DISPC manager size should also be
configured at 480x864 right?
Yep. When we use the panel rotation, from OMAP's point of view the panel
resolution has changed.
We seem to be setting the manager timings only once when DSI is enabled.
After that, setting rotation doesn't impact manager size.
Hmm, previously the mgr size was set before each update. I wonder if
that code has been dropped, probably because we removed the support for
partial updates at one point. Without partial updates, the size stays
the same, except obviously with rotation. I think I just forgot about
rotation at that time.
I am asking this to understand if we need to keep resolution as a
separate parameter than timings. That is, timings represents the initial
width and height of the panel, and resolution represents the current
width and height of the panel.
I'm not sure. I think that OMAP doesn't really need to know about the
initial resolution. It doesn't really matter from OMAP's point of view.
I think I originally kept timings and resolution separately, and the
idea was that timings represent the panel's timings, i.e. how it updates
the screen from its own memory. And resolution represents the usable
resolution, from OMAP's point of view.
While I haven't seen such a cmd mode panel, there could be a command
sent to the panel to configure its timings. For this we need real
timings, not the rotated resolution.
However, even in that case the DISPC doesn't need to know about those
timings, they would be handled by the panel driver (which could,
perhaps, reconfigure the DSI bus speed to match the new timings). So I
think that inside omapdss, we don't need separate timings and resolution
for DSI cmd mode panels.
Tomi
On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
quoted
This series tries to make interface drivers less dependent on omap_dss_device
which represents a panel/device connected to that interface. The current way of
configuring an interface is to populate the panel's omap_dss_device instance
with parameters common to the panel and the interface, they are either populated
in the board file, or in the panel driver. Panel timings, number of lanes
connected to interface, and pixel format are examples of such parameters, these
are then extracted by the interface driver to configure itself.
The series looks good. I had only a few comments to make, but obviously
this needs quite a bit of testing. I'll try it out.
One thing I'm not sure about is whether these new functions should be
aware of the state of the output. For example, if we call set_timings()
with DSI video mode which is already enabled, the timings won't really
take any impact.
Similar issues would occur when we try to make other ops like
set_data_lines() or set_pixel_format(). These need to be called before
the output is enabled. I was wondering if we would need to add
intelligence here to make panel drivers less likely to make mistakes.
Archit
From: Tomi Valkeinen <hidden> Date: 2012-08-08 06:25:58
On Wed, 2012-08-08 at 11:35 +0530, Archit Taneja wrote:
On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
quoted
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
quoted
This series tries to make interface drivers less dependent on omap_dss_device
which represents a panel/device connected to that interface. The current way of
configuring an interface is to populate the panel's omap_dss_device instance
with parameters common to the panel and the interface, they are either populated
in the board file, or in the panel driver. Panel timings, number of lanes
connected to interface, and pixel format are examples of such parameters, these
are then extracted by the interface driver to configure itself.
The series looks good. I had only a few comments to make, but obviously
this needs quite a bit of testing. I'll try it out.
One thing I'm not sure about is whether these new functions should be
aware of the state of the output. For example, if we call set_timings()
with DSI video mode which is already enabled, the timings won't really
take any impact.
Similar issues would occur when we try to make other ops like
set_data_lines() or set_pixel_format(). These need to be called before
the output is enabled. I was wondering if we would need to add
intelligence here to make panel drivers less likely to make mistakes.
Hmm, true. It'd be nice if the functions returned -EBUSY if the
operation cannot be done while the output is enabled.
We have the dssdev->state, but we should get rid of that (or leave it to
panel drivers). It'd be good if the output drivers know whether the
output is enabled or not. I think this data is already tracked by
apply.c. It's about ovl managers, but I think that's practically the
same as output.
Calling dss_mgr_enable() will set mp->enabled = true, which could be
returned via dss_mgr_is_enabled() or such.
Then again, it wouldn't be many lines of codes to track the enable-state
in each output driver. So if we have any suspicions that mp->enabled
doesn't quite work for, say, dsi, we could just add a private "enabled"
member to dsi. But I don't right away see why dss_mgr_is_enabled()
wouldn't work.
Tomi
On Wednesday 08 August 2012 11:45 AM, Tomi Valkeinen wrote:
On Wed, 2012-08-08 at 11:27 +0530, Archit Taneja wrote:
quoted
I am a bit unclear about resolution when it comes to command mode panels.
Right, it's a bit confusing. And I'm not 100% sure how to manage the
rotation.
quoted
For command mode panels, we can perform rotation at the panel side. That
is, the panel refreshes itself by fetching pixels from it's buffer in a
rotated way. Is that right?
Yes. Well, actually I think the panel stores the pixels in rotated
manner when it receives them from OMAP, but it's practically the same.
One thing to realize is that this kind of rotation is a bit limited:
because there's only one buffer, OMAP will write pixels to the buffers
at the same time as the panel shows them. When rotating, this leads to
tearing.
If the panel has double buffer, that solves the problem, but I haven't
seen such panels. Another option is to update the panel in two parts,
like N9 does, but that's timing sensitive and a bit tricky.
quoted
If the original resolution is 864x480, and we set rotation at panel side
to make the rotation 480x864, the DISPC manager size should also be
configured at 480x864 right?
Yep. When we use the panel rotation, from OMAP's point of view the panel
resolution has changed.
quoted
We seem to be setting the manager timings only once when DSI is enabled.
After that, setting rotation doesn't impact manager size.
Hmm, previously the mgr size was set before each update. I wonder if
that code has been dropped, probably because we removed the support for
partial updates at one point. Without partial updates, the size stays
the same, except obviously with rotation. I think I just forgot about
rotation at that time.
I tried out rotation on Taal, and it only works for 180 degrees(and 0 of
course), 90 and 270 result in no output. I'll add a
dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it,
but someone would need to reconfigure the connected overlays too before
trying out an update.
quoted
I am asking this to understand if we need to keep resolution as a
separate parameter than timings. That is, timings represents the initial
width and height of the panel, and resolution represents the current
width and height of the panel.
I'm not sure. I think that OMAP doesn't really need to know about the
initial resolution. It doesn't really matter from OMAP's point of view.
I think I originally kept timings and resolution separately, and the
idea was that timings represent the panel's timings, i.e. how it updates
the screen from its own memory. And resolution represents the usable
resolution, from OMAP's point of view.
While I haven't seen such a cmd mode panel, there could be a command
sent to the panel to configure its timings. For this we need real
timings, not the rotated resolution.
However, even in that case the DISPC doesn't need to know about those
timings, they would be handled by the panel driver (which could,
perhaps, reconfigure the DSI bus speed to match the new timings). So I
think that inside omapdss, we don't need separate timings and resolution
for DSI cmd mode panels.
On Wednesday 08 August 2012 11:55 AM, Tomi Valkeinen wrote:
On Wed, 2012-08-08 at 11:35 +0530, Archit Taneja wrote:
quoted
On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
quoted
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
quoted
This series tries to make interface drivers less dependent on omap_dss_device
which represents a panel/device connected to that interface. The current way of
configuring an interface is to populate the panel's omap_dss_device instance
with parameters common to the panel and the interface, they are either populated
in the board file, or in the panel driver. Panel timings, number of lanes
connected to interface, and pixel format are examples of such parameters, these
are then extracted by the interface driver to configure itself.
The series looks good. I had only a few comments to make, but obviously
this needs quite a bit of testing. I'll try it out.
One thing I'm not sure about is whether these new functions should be
aware of the state of the output. For example, if we call set_timings()
with DSI video mode which is already enabled, the timings won't really
take any impact.
Similar issues would occur when we try to make other ops like
set_data_lines() or set_pixel_format(). These need to be called before
the output is enabled. I was wondering if we would need to add
intelligence here to make panel drivers less likely to make mistakes.
Hmm, true. It'd be nice if the functions returned -EBUSY if the
operation cannot be done while the output is enabled.
We have the dssdev->state, but we should get rid of that (or leave it to
panel drivers). It'd be good if the output drivers know whether the
output is enabled or not. I think this data is already tracked by
apply.c. It's about ovl managers, but I think that's practically the
same as output.
Calling dss_mgr_enable() will set mp->enabled = true, which could be
returned via dss_mgr_is_enabled() or such.
Then again, it wouldn't be many lines of codes to track the enable-state
in each output driver. So if we have any suspicions that mp->enabled
doesn't quite work for, say, dsi, we could just add a private "enabled"
member to dsi. But I don't right away see why dss_mgr_is_enabled()
wouldn't work.
I think we had discussed previously that it may not the best idea to see
if a manager is enabled via mp->enabled as it's always possible that it
changes afterwards. Same for any other parameter in APPLY's private
data. This was the reason why we passed privtate data to DISPC functions
rather than creating apply helper functions which return the value of a
private data. For example, we pass manager timings to dispc_ovl_setup(),
instead of DISPC using a function like dss_mgr_get_timings().
I also don't see why dss_mgr_is_enabled() wouldn't work. The only places
where the manager's state will change are the output's enable and
disable ops. The mutex maintained by the output would ensure
sequential-ity between the output's enable() and set_timings() op, and
hence ensure the manager's state we see is fine.
If we manage the 'enabled' state for each output interface, we would be
a bit more consistent with respect to other parameters. For example,
timings is maintained by both manager and the output. Also, if we need
to separate out manager configurations from outputs in the future, it
would probably be better for the output to query it's own state rather
than depending on the manager, which could be configured either earlier
or later.
Archit
From: Tomi Valkeinen <hidden> Date: 2012-08-08 07:10:47
On Wed, 2012-08-08 at 11:59 +0530, Archit Taneja wrote:
I tried out rotation on Taal, and it only works for 180 degrees(and 0 of
course), 90 and 270 result in no output. I'll add a
dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it,
but someone would need to reconfigure the connected overlays too before
trying out an update.
Right, but that's something omapdss/panel cannot do, it must be done by
the user. The same problem is there with changing, say, DPI mode also.
Btw, can you separate smaller cleanups/fixes to another patch series, to
make this series even slightly smaller? I think at least the first
patches in this series are quite separate, and the rotation fix is also.
Tomi
From: Tomi Valkeinen <hidden> Date: 2012-08-08 07:27:15
On Wed, 2012-08-08 at 12:17 +0530, Archit Taneja wrote:
On Wednesday 08 August 2012 11:55 AM, Tomi Valkeinen wrote:
quoted
On Wed, 2012-08-08 at 11:35 +0530, Archit Taneja wrote:
quoted
On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
quoted
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
quoted
This series tries to make interface drivers less dependent on omap_dss_device
which represents a panel/device connected to that interface. The current way of
configuring an interface is to populate the panel's omap_dss_device instance
with parameters common to the panel and the interface, they are either populated
in the board file, or in the panel driver. Panel timings, number of lanes
connected to interface, and pixel format are examples of such parameters, these
are then extracted by the interface driver to configure itself.
The series looks good. I had only a few comments to make, but obviously
this needs quite a bit of testing. I'll try it out.
One thing I'm not sure about is whether these new functions should be
aware of the state of the output. For example, if we call set_timings()
with DSI video mode which is already enabled, the timings won't really
take any impact.
Similar issues would occur when we try to make other ops like
set_data_lines() or set_pixel_format(). These need to be called before
the output is enabled. I was wondering if we would need to add
intelligence here to make panel drivers less likely to make mistakes.
Hmm, true. It'd be nice if the functions returned -EBUSY if the
operation cannot be done while the output is enabled.
We have the dssdev->state, but we should get rid of that (or leave it to
panel drivers). It'd be good if the output drivers know whether the
output is enabled or not. I think this data is already tracked by
apply.c. It's about ovl managers, but I think that's practically the
same as output.
Calling dss_mgr_enable() will set mp->enabled = true, which could be
returned via dss_mgr_is_enabled() or such.
Then again, it wouldn't be many lines of codes to track the enable-state
in each output driver. So if we have any suspicions that mp->enabled
doesn't quite work for, say, dsi, we could just add a private "enabled"
member to dsi. But I don't right away see why dss_mgr_is_enabled()
wouldn't work.
I think we had discussed previously that it may not the best idea to see
if a manager is enabled via mp->enabled as it's always possible that it
changes afterwards. Same for any other parameter in APPLY's private
data. This was the reason why we passed privtate data to DISPC functions
rather than creating apply helper functions which return the value of a
private data. For example, we pass manager timings to dispc_ovl_setup(),
instead of DISPC using a function like dss_mgr_get_timings().
I think that's slightly different problem. The dispc case has an issue
with locking. If dispc_ovl_setup() is called with the apply's spinlock
taken, neither dispc_ovl_setup() nor dss_mgr_get_timings() can take the
lock. But if dispc_ovl_setup() is called from somewhere else, it should
take the lock. Also, if dispc_ovl_setup() would call a function in
apply, it'd be calling "upwards" to a higher level component.
With the output driver calling apply, none of those problems is present,
I believe.
I also don't see why dss_mgr_is_enabled() wouldn't work. The only places
where the manager's state will change are the output's enable and
disable ops. The mutex maintained by the output would ensure
sequential-ity between the output's enable() and set_timings() op, and
hence ensure the manager's state we see is fine.
If we manage the 'enabled' state for each output interface, we would be
a bit more consistent with respect to other parameters. For example,
timings is maintained by both manager and the output. Also, if we need
to separate out manager configurations from outputs in the future, it
would probably be better for the output to query it's own state rather
than depending on the manager, which could be configured either earlier
or later.
Two things that came to my mind:
If the output driver uses dss_mgr_is_enabled(), if both DPI and DSI
output drivers use the same manager, they'd both see themselves as
enabled. Of course only one can work at a time, so I'm not sure if
that's a practical problem. And if we had some kind of link between the
mgr and the output driver this would not be an issue.
The second thing is that we're not strictly required to have DISPC
connected to DSI or RFBI. We could use CPU/sDMA to output the image.
This is quite theoretical, though.
So, I think using dss_mgr_is_enabled() would work, but I'm still not
100% sure...
Well, perhaps the code should be such that dss_mgr_is_enabled() is used
to see if the mgr is enabled, not if the output is enabled. What I mean
with this is that if, say, set_data_lines() calls dispc to set the data
lines, we are really interested in if the dispc's mgr is enabled, not if
the DSI is enabled.
And if some other function changes DSI configuration (but doesn't touch
dispc), then we're not really interested in if the mgr is enabled, but
if the DSI is enabled.
That's a bit more complex than using only dss_mgr_is_enabled() or using
only output specific enable-flag, but I think it's more correct. In
DPI's case only dss_mgr_is_enabled() is probably needed. For DSI we may
need a separate private enable-flag.
Tomi
On Wednesday 08 August 2012 12:40 PM, Tomi Valkeinen wrote:
On Wed, 2012-08-08 at 11:59 +0530, Archit Taneja wrote:
quoted
I tried out rotation on Taal, and it only works for 180 degrees(and 0 of
course), 90 and 270 result in no output. I'll add a
dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it,
but someone would need to reconfigure the connected overlays too before
trying out an update.
Right, but that's something omapdss/panel cannot do, it must be done by
the user. The same problem is there with changing, say, DPI mode also.
Btw, can you separate smaller cleanups/fixes to another patch series, to
make this series even slightly smaller? I think at least the first
patches in this series are quite separate, and the rotation fix is also.
On Wednesday 08 August 2012 12:57 PM, Tomi Valkeinen wrote:
On Wed, 2012-08-08 at 12:17 +0530, Archit Taneja wrote:
quoted
On Wednesday 08 August 2012 11:55 AM, Tomi Valkeinen wrote:
quoted
On Wed, 2012-08-08 at 11:35 +0530, Archit Taneja wrote:
quoted
On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
quoted
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
quoted
This series tries to make interface drivers less dependent on omap_dss_device
which represents a panel/device connected to that interface. The current way of
configuring an interface is to populate the panel's omap_dss_device instance
with parameters common to the panel and the interface, they are either populated
in the board file, or in the panel driver. Panel timings, number of lanes
connected to interface, and pixel format are examples of such parameters, these
are then extracted by the interface driver to configure itself.
The series looks good. I had only a few comments to make, but obviously
this needs quite a bit of testing. I'll try it out.
One thing I'm not sure about is whether these new functions should be
aware of the state of the output. For example, if we call set_timings()
with DSI video mode which is already enabled, the timings won't really
take any impact.
Similar issues would occur when we try to make other ops like
set_data_lines() or set_pixel_format(). These need to be called before
the output is enabled. I was wondering if we would need to add
intelligence here to make panel drivers less likely to make mistakes.
Hmm, true. It'd be nice if the functions returned -EBUSY if the
operation cannot be done while the output is enabled.
We have the dssdev->state, but we should get rid of that (or leave it to
panel drivers). It'd be good if the output drivers know whether the
output is enabled or not. I think this data is already tracked by
apply.c. It's about ovl managers, but I think that's practically the
same as output.
Calling dss_mgr_enable() will set mp->enabled = true, which could be
returned via dss_mgr_is_enabled() or such.
Then again, it wouldn't be many lines of codes to track the enable-state
in each output driver. So if we have any suspicions that mp->enabled
doesn't quite work for, say, dsi, we could just add a private "enabled"
member to dsi. But I don't right away see why dss_mgr_is_enabled()
wouldn't work.
I think we had discussed previously that it may not the best idea to see
if a manager is enabled via mp->enabled as it's always possible that it
changes afterwards. Same for any other parameter in APPLY's private
data. This was the reason why we passed privtate data to DISPC functions
rather than creating apply helper functions which return the value of a
private data. For example, we pass manager timings to dispc_ovl_setup(),
instead of DISPC using a function like dss_mgr_get_timings().
I think that's slightly different problem. The dispc case has an issue
with locking. If dispc_ovl_setup() is called with the apply's spinlock
taken, neither dispc_ovl_setup() nor dss_mgr_get_timings() can take the
lock. But if dispc_ovl_setup() is called from somewhere else, it should
take the lock. Also, if dispc_ovl_setup() would call a function in
apply, it'd be calling "upwards" to a higher level component.
With the output driver calling apply, none of those problems is present,
I believe.
quoted
I also don't see why dss_mgr_is_enabled() wouldn't work. The only places
where the manager's state will change are the output's enable and
disable ops. The mutex maintained by the output would ensure
sequential-ity between the output's enable() and set_timings() op, and
hence ensure the manager's state we see is fine.
If we manage the 'enabled' state for each output interface, we would be
a bit more consistent with respect to other parameters. For example,
timings is maintained by both manager and the output. Also, if we need
to separate out manager configurations from outputs in the future, it
would probably be better for the output to query it's own state rather
than depending on the manager, which could be configured either earlier
or later.
Two things that came to my mind:
If the output driver uses dss_mgr_is_enabled(), if both DPI and DSI
output drivers use the same manager, they'd both see themselves as
enabled. Of course only one can work at a time, so I'm not sure if
that's a practical problem. And if we had some kind of link between the
mgr and the output driver this would not be an issue.
The second thing is that we're not strictly required to have DISPC
connected to DSI or RFBI. We could use CPU/sDMA to output the image.
This is quite theoretical, though.
So, I think using dss_mgr_is_enabled() would work, but I'm still not
100% sure...
Well, perhaps the code should be such that dss_mgr_is_enabled() is used
to see if the mgr is enabled, not if the output is enabled. What I mean
with this is that if, say, set_data_lines() calls dispc to set the data
lines, we are really interested in if the dispc's mgr is enabled, not if
the DSI is enabled.
And if some other function changes DSI configuration (but doesn't touch
dispc), then we're not really interested in if the mgr is enabled, but
if the DSI is enabled.
That's a bit more complex than using only dss_mgr_is_enabled() or using
only output specific enable-flag, but I think it's more correct. In
DPI's case only dss_mgr_is_enabled() is probably needed. For DSI we may
need a separate private enable-flag.
Okay, one thing which I want to align on is that most of these functions
don't really do the actual configurations. That is, they'll just update
the private data, and the actual configuration will only happen on enable.
We would want set_timings() op to have a direct impact. But we wouldn't
want the same for setting the data lines, that could be clubbed with
other configurations at enable. That's okay, right?
Archit
From: Tomi Valkeinen <hidden> Date: 2012-08-08 08:13:00
On Wed, 2012-08-08 at 13:29 +0530, Archit Taneja wrote:
Okay, one thing which I want to align on is that most of these functions
don't really do the actual configurations. That is, they'll just update
the private data, and the actual configuration will only happen on enable.
We would want set_timings() op to have a direct impact. But we wouldn't
want the same for setting the data lines, that could be clubbed with
other configurations at enable. That's okay, right?
I'm not sure we want/need set_timings to have direct impact. Changing
the timings on the fly has some problems, like the output size changing
to smaller than the overlays, and we perhaps may need to adjust the
clock dividers (dispc's, DSI PLL's or PRCM's).
It feels just much easier and safer to require that the mgr is disabled
when these changes are made. And as far as I can see, there shouldn't be
any need to change the timings via the shadow registers, as quickly as
possible and during vblank...
This makes me also think that if the output related settings can only be
changed when the output is off, the apply mechanism is not really needed
at all for these. Not that it causes any harm, but just a point I
realized.
Tomi
From: Tomi Valkeinen <hidden> Date: 2012-08-08 08:48:05
On Wed, 2012-08-08 at 14:08 +0530, Archit Taneja wrote:
On Wednesday 08 August 2012 01:43 PM, Tomi Valkeinen wrote:
quoted
On Wed, 2012-08-08 at 13:29 +0530, Archit Taneja wrote:
quoted
Okay, one thing which I want to align on is that most of these functions
don't really do the actual configurations. That is, they'll just update
the private data, and the actual configuration will only happen on enable.
We would want set_timings() op to have a direct impact. But we wouldn't
want the same for setting the data lines, that could be clubbed with
other configurations at enable. That's okay, right?
I'm not sure we want/need set_timings to have direct impact. Changing
the timings on the fly has some problems, like the output size changing
to smaller than the overlays, and we perhaps may need to adjust the
clock dividers (dispc's, DSI PLL's or PRCM's).
It feels just much easier and safer to require that the mgr is disabled
when these changes are made. And as far as I can see, there shouldn't be
any need to change the timings via the shadow registers, as quickly as
possible and during vblank...
That makes sense. But currently set_timings for DPI has a direct impact.
HDMI/VENC/SDI take the easier route of disabling and enabling the interface.
I agree it's safer and easier to make sure things are disabled first,
but maybe it's good to have the capability set hdmi timings on the fly
in the future, it would make the switch faster, same goes for reading edid.
When do we need to switch mode quickly? Reading edid should not require
disabling the output for sure.
HDMI is a bit broken currently, though. I think we first enable the
whole stuff, including video output using VGA, then we read EDID, then
we change the mode.
We should just enable enough of HDMI to be able to read EDID, and start
the video output with the correct mode. This needs some restructuring of
the driver, though. I tried it once quickly, but it turned out not to be
trivial.
What I meant to ask was whether we should do the same for something like
dpi_set_data_lines(), that is, disable dpi, update the data_lines
private data with a new value, and enable dpi again.
Hmm, I think it's better to leave disabling and enabling the output to
the panel driver. So when the panel driver wants to use
dpi_set_data_lines(), it needs to first disable the DPI output. If it
doesn't, dpi_set_data_lines() returns -EBUSY.
Otherwise if the panel driver does something like:
dpi_set_foo()
dpi_set_bar()
Both of those could first disable output, change setting, enable output.
Instead the panel should first disable, then call those, and then
enable.
Tomi
On Wednesday 08 August 2012 01:43 PM, Tomi Valkeinen wrote:
On Wed, 2012-08-08 at 13:29 +0530, Archit Taneja wrote:
quoted
Okay, one thing which I want to align on is that most of these functions
don't really do the actual configurations. That is, they'll just update
the private data, and the actual configuration will only happen on enable.
We would want set_timings() op to have a direct impact. But we wouldn't
want the same for setting the data lines, that could be clubbed with
other configurations at enable. That's okay, right?
I'm not sure we want/need set_timings to have direct impact. Changing
the timings on the fly has some problems, like the output size changing
to smaller than the overlays, and we perhaps may need to adjust the
clock dividers (dispc's, DSI PLL's or PRCM's).
It feels just much easier and safer to require that the mgr is disabled
when these changes are made. And as far as I can see, there shouldn't be
any need to change the timings via the shadow registers, as quickly as
possible and during vblank...
That makes sense. But currently set_timings for DPI has a direct impact.
HDMI/VENC/SDI take the easier route of disabling and enabling the interface.
I agree it's safer and easier to make sure things are disabled first,
but maybe it's good to have the capability set hdmi timings on the fly
in the future, it would make the switch faster, same goes for reading edid.
What I meant to ask was whether we should do the same for something like
dpi_set_data_lines(), that is, disable dpi, update the data_lines
private data with a new value, and enable dpi again.
This makes me also think that if the output related settings can only be
changed when the output is off, the apply mechanism is not really needed
at all for these. Not that it causes any harm, but just a point I
realized.
Hmm, unfortunately you are right. It's still good to have all the DISPC
writes only in APPLY though, and it gives us the option to do some
operation on the fly if needed in the future.
Archit
On Wednesday 08 August 2012 02:18 PM, Tomi Valkeinen wrote:
On Wed, 2012-08-08 at 14:08 +0530, Archit Taneja wrote:
quoted
On Wednesday 08 August 2012 01:43 PM, Tomi Valkeinen wrote:
quoted
On Wed, 2012-08-08 at 13:29 +0530, Archit Taneja wrote:
quoted
Okay, one thing which I want to align on is that most of these functions
don't really do the actual configurations. That is, they'll just update
the private data, and the actual configuration will only happen on enable.
We would want set_timings() op to have a direct impact. But we wouldn't
want the same for setting the data lines, that could be clubbed with
other configurations at enable. That's okay, right?
I'm not sure we want/need set_timings to have direct impact. Changing
the timings on the fly has some problems, like the output size changing
to smaller than the overlays, and we perhaps may need to adjust the
clock dividers (dispc's, DSI PLL's or PRCM's).
It feels just much easier and safer to require that the mgr is disabled
when these changes are made. And as far as I can see, there shouldn't be
any need to change the timings via the shadow registers, as quickly as
possible and during vblank...
That makes sense. But currently set_timings for DPI has a direct impact.
HDMI/VENC/SDI take the easier route of disabling and enabling the interface.
I agree it's safer and easier to make sure things are disabled first,
but maybe it's good to have the capability set hdmi timings on the fly
in the future, it would make the switch faster, same goes for reading edid.
When do we need to switch mode quickly? Reading edid should not require
disabling the output for sure.
I think I'm just finding excuses to find a use for my work done in
APPLYing manager related registers.
You are right about edid. Changing HDMI timings take a couple of seconds
now, I was wondering how much that has to do with us completely
disabling/enabling hdmi. it may be just the slowness of the monitors
which causes this.
HDMI is a bit broken currently, though. I think we first enable the
whole stuff, including video output using VGA, then we read EDID, then
we change the mode.
We should just enable enough of HDMI to be able to read EDID, and start
the video output with the correct mode. This needs some restructuring of
the driver, though. I tried it once quickly, but it turned out not to be
trivial.
Right. Most likely this restructuring would allow us to modify only the
hdmi timings part when setting a new timing. We could check how much
time we save then :)
quoted
What I meant to ask was whether we should do the same for something like
dpi_set_data_lines(), that is, disable dpi, update the data_lines
private data with a new value, and enable dpi again.
Hmm, I think it's better to leave disabling and enabling the output to
the panel driver. So when the panel driver wants to use
dpi_set_data_lines(), it needs to first disable the DPI output. If it
doesn't, dpi_set_data_lines() returns -EBUSY.
Otherwise if the panel driver does something like:
dpi_set_foo()
dpi_set_bar()
Both of those could first disable output, change setting, enable output.
Instead the panel should first disable, then call those, and then
enable.
This is a follow up series. The orginal one can be seen here:
http://marc.info/?l=linux-omap&m4381744304672&w=2
Changes in v2:
- Removed misc fixes out of this set
- Not trying to optimize SDI when setting new timings, using the old strategy
- Added a function for setting size for DSI command mode panels
- Added a fix which ensures the manager sizes is correctly set for rotation in
command mode panels.
The tree/branch can be found here:
git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git pass_timings_interface_2
Archit Taneja (13):
OMAPDSS: DPI: Maintain our own timings field in driver data
OMAPDSS: DPI displays: Take care of panel timings in the driver
itself
OMAPDSS: DSI: Maintain own copy of timings in driver data
OMAPDSS: DSI: Add function to set panel size for command mode panels
OMAPDSS: DSI: Update manager timings on a manual update
OMAPDSS: HDMI: Use our own omap_video_timings field when setting
interface timings
OMAPDSS: HDMI: Add a get_timing function for HDMI interface
OMAPDSS: HDMI: Add locking for hdmi interface get/set timing
functions
OMAPDSS: SDI: Create a function to set timings
OMAPDSS: SDI: Maintain our own timings field in driver data
OMAPDSS: VENC: Split VENC into interface and panel driver
OMAPDSS: VENC: Maintain our own timings field in driver data
OMAPDSS: VENC: Add a get_timing function for VENC interface
drivers/video/omap2/displays/panel-acx565akm.c | 13 +-
drivers/video/omap2/displays/panel-generic-dpi.c | 6 +-
.../omap2/displays/panel-lgphilips-lb035q02.c | 2 +
.../omap2/displays/panel-nec-nl8048hl11-01b.c | 2 +
drivers/video/omap2/displays/panel-picodlp.c | 3 +
.../video/omap2/displays/panel-sharp-ls037v7dw01.c | 2 +
drivers/video/omap2/displays/panel-taal.c | 14 ++
drivers/video/omap2/displays/panel-tfp410.c | 5 +-
.../video/omap2/displays/panel-tpo-td043mtea1.c | 6 +-
drivers/video/omap2/dss/Makefile | 2 +-
drivers/video/omap2/dss/dpi.c | 12 +-
drivers/video/omap2/dss/dsi.c | 86 +++++---
drivers/video/omap2/dss/dss.h | 17 +-
drivers/video/omap2/dss/hdmi.c | 55 +++--
drivers/video/omap2/dss/hdmi_panel.c | 12 +-
drivers/video/omap2/dss/sdi.c | 20 +-
drivers/video/omap2/dss/venc.c | 233 ++++++--------------
drivers/video/omap2/dss/venc_panel.c | 231 +++++++++++++++++++
include/video/omapdss.h | 9 +-
19 files changed, 490 insertions(+), 240 deletions(-)
create mode 100644 drivers/video/omap2/dss/venc_panel.c
--
1.7.9.5
The DPI driver currently relies on the timings in omap_dss_device struct to
configure the DISPC accordingly. This makes the DPI interface driver dependent
on the omap_dss_device struct.
Make the DPI driver data maintain it's own timings field. The panel driver is
expected to call dpi_set_timings()(renamed to omapdss_dpi_set_timings) to set
these timings before the panel is enabled.
In the set_timings() op, we still ensure that the omap_dss_device timings
(dssdev->panel.timings) are configured. This will later be configured only by
the DPI panel drivers.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-generic-dpi.c | 4 +++-
.../omap2/displays/panel-lgphilips-lb035q02.c | 2 ++
.../omap2/displays/panel-nec-nl8048hl11-01b.c | 2 ++
drivers/video/omap2/displays/panel-picodlp.c | 3 +++
.../video/omap2/displays/panel-sharp-ls037v7dw01.c | 2 ++
drivers/video/omap2/displays/panel-tfp410.c | 4 +++-
.../video/omap2/displays/panel-tpo-td043mtea1.c | 4 +++-
drivers/video/omap2/dss/dpi.c | 11 +++++++----
include/video/omapdss.h | 4 ++--
9 files changed, 27 insertions(+), 9 deletions(-)
@@ -138,7 +139,7 @@ static int dpi_set_dispc_clk(unsigned long pck_req, unsigned long *fck,staticintdpi_set_mode(structomap_dss_device*dssdev){-structomap_video_timings*t=&dssdev->panel.timings;+structomap_video_timings*t=&dpi.timings;intlck_div=0,pck_div=0;unsignedlongfck=0;unsignedlongpck;
The timings maintained in omap_dss_device(dssdev->panel.timings) should be
maintained by the panel driver itself. It's the panel drivers responsibility
to update it if a new set of timings is to be configured. The DPI interface
driver shouldn't be responsible of updating the panel timings, it's responsible
of maintianing it's own copy of timings.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-generic-dpi.c | 2 ++
drivers/video/omap2/displays/panel-tfp410.c | 1 +
.../video/omap2/displays/panel-tpo-td043mtea1.c | 2 ++
drivers/video/omap2/dss/dpi.c | 1 -
4 files changed, 5 insertions(+), 1 deletion(-)
The DSI driver currently relies on the timings in omap_dss_device struct to
configure the DISPC and DSI blocks accordingly. This makes the DSI interface
driver dependent on the omap_dss_device struct.
Make the DSI driver data maintain it's own timings field. A DSI video mode panel
driver is expected to call omapdss_dsi_set_timings() to set these timings before
the panel is enabled.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dsi.c | 58 +++++++++++++++++++++++++----------------
include/video/omapdss.h | 2 ++
2 files changed, 38 insertions(+), 22 deletions(-)
DSI command mode panels don't need to configure a full set of timings to
configure DSI, they only require the width and the height of the panel in
pixels.
Use omapdss_dsi_set_size for command mode panels, omapdss_dsi_set_timings is
meant for video mode panels. When performing rotation via chaning the address
mode of the panel, we would need to swap width and height when doing 90 or 270
rotation. Make sure that omapdss_dsi_set_size() makes the new width and height
visible to DSI.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-taal.c | 14 ++++++++++++++
drivers/video/omap2/dss/dsi.c | 23 ++++++++++++++++-------
include/video/omapdss.h | 1 +
3 files changed, 31 insertions(+), 7 deletions(-)
During a command mode update using DISPC video port, we may need to swap the
connected overlay manager's width and height when 90 or 270 degree rotation is
done via the panel by changing it's address mode.
Call dss_mgr_set_timings() in update_screen_dispc() before starting the manager
update. The new manager size is updated in the 'timings' field of DSI driver's
private data via omapdss_dsi_set_size(). A panel driver is expected to call this
when performing rotation.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dsi.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
@@ -4337,7 +4340,7 @@ int omap_dsi_update(struct omap_dss_device *dssdev, int channel,dsi->update_bytes=dw*dh*dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt)/8;#endif-dsi_update_screen_dispc(dssdev,dw,dh);+dsi_update_screen_dispc(dssdev);return0;}
The hdmi driver currently updates only the 'code' member of hdmi_config when
the op omapdss_hdmi_display_set_timing() is called by the hdmi panel driver.
The 'timing' field of hdmi_config is updated only when hdmi_power_on is called.
It makes more sense to configure the whole hdmi_config field in the set_timing
op called by the panel driver. This way, we don't need to call both functions
to ensure that our hdmi_config is configured correctly. Also, we don't need to
calculate hdmi_config during hdmi_power_on, or rely on the omap_video_timings
in the panel's omap_dss_device struct.
A default timing is now configured in hdmi's probe if the panel driver doesn't
set any timing, or doesn't set a valid timing before enabling the panel. Also,
when setting manager timings, use the omap_video_timing calculated by
hdmi_get_timings(), this returns the timings as specified in the CEA/VESA
tables, don't use the one provided by the panel driver directly.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dss.h | 3 ++-
drivers/video/omap2/dss/hdmi.c | 41 +++++++++++++++++-----------------
drivers/video/omap2/dss/hdmi_panel.c | 2 +-
3 files changed, 23 insertions(+), 23 deletions(-)
@@ -462,7 +462,6 @@ static int hdmi_power_on(struct omap_dss_device *dssdev){conststructomapdss_clock_config*clks;intr;-conststructhdmi_config*timing;structomap_video_timings*p;unsignedlongphy;
@@ -472,22 +471,10 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)dss_mgr_disable(dssdev->manager);-p=&dssdev->panel.timings;+p=&hdmi.ip_data.cfg.timings;-DSSDBG("hdmi_power_on x_res= %d y_res = %d\n",-dssdev->panel.timings.x_res,-dssdev->panel.timings.y_res);+DSSDBG("hdmi_power_on x_res= %d y_res = %d\n",p->x_res,p->y_res);-timing=hdmi_get_timings();-if(timing=NULL){-/* HDMI code 4 corresponds to 640 * 480 VGA */-hdmi.ip_data.cfg.cm.code=4;-/* DVI mode 1 corresponds to HDMI 0 to DVI */-hdmi.ip_data.cfg.cm.mode=HDMI_DVI;-hdmi.ip_data.cfg=vesa_timings[0];-}else{-hdmi.ip_data.cfg=*timing;-}phy=p->pixel_clock;hdmi_compute_pll(phy,&hdmi.ip_data.pll_data);
@@ -525,7 +512,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)dispc_enable_gamma_table(0);/* tv size */-dss_mgr_set_timings(dssdev->manager,&dssdev->panel.timings);+dss_mgr_set_timings(dssdev->manager,p);r=hdmi.ip_data.ops->video_enable(&hdmi.ip_data);if(r)
@@ -571,13 +558,18 @@ int omapdss_hdmi_display_check_timing(struct omap_dss_device *dssdev,}-voidomapdss_hdmi_display_set_timing(structomap_dss_device*dssdev)+voidomapdss_hdmi_display_set_timing(structomap_dss_device*dssdev,+structomap_video_timings*timings){structhdmi_cmcm;+conststructhdmi_config*t;++cm=hdmi_get_code(timings);+hdmi.ip_data.cfg.cm=cm;-cm=hdmi_get_code(&dssdev->panel.timings);-hdmi.ip_data.cfg.cm.code=cm.code;-hdmi.ip_data.cfg.cm.mode=cm.mode;+t=hdmi_get_timings();+if(t!=NULL)+hdmi.ip_data.cfg=*t;if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){intr;
@@ -588,7 +580,7 @@ void omapdss_hdmi_display_set_timing(struct omap_dss_device *dssdev)if(r)DSSERR("failed to power on device\n");}else{-dss_mgr_set_timings(dssdev->manager,&dssdev->panel.timings);+dss_mgr_set_timings(dssdev->manager,&t->timings);}}
@@ -933,6 +925,13 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)hdmi.ip_data.core_av_offset=HDMI_CORE_AV;hdmi.ip_data.pll_offset=HDMI_PLLCTRL;hdmi.ip_data.phy_offset=HDMI_PHY;++/*+*initializehdmitimingstodefaultvalue:+*HDMIcode4(VGA)andHDMImode1(DVI)+*/+hdmi.ip_data.cfg=vesa_timings[0];+mutex_init(&hdmi.ip_data.lock);hdmi_panel_init();
Add function omapdss_hdmi_display_get_timing() which returns the timings
maintained by the HDMI interface driver in it's hdmi_config field. This
prevents the need for the panel driver to configure default timings in it's
probe.
This function is just intended to be used once during the panel driver's probe.
It makes sense for those interfaces which can be configured to a default timing.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dss.h | 2 ++
drivers/video/omap2/dss/hdmi.c | 6 ++++++
drivers/video/omap2/dss/hdmi_panel.c | 10 +++++-----
3 files changed, 13 insertions(+), 5 deletions(-)
The hdmi interface driver exposes functions to the hdmi panel driver to
get and configure the interface timings maintained by the hdmi driver.
These timings(stored in hdmi.ip_data.cfg) should be protected by the hdmi lock
to ensure they are called sequentially, this is similar to how hdmi enable and
disable functions need locking.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/hdmi.c | 8 ++++++++
1 file changed, 8 insertions(+)
Create function omapdss_sdi_set_timings(). Configuring new timings is done the
same way as before, SDI is disabled, and re-enabled with the new timings in
dssdev. This just moves the code from the panel drivers to the SDI driver.
The panel drivers shouldn't be aware of how SDI manages to configure a new set
of timings. This should be taken care of by the SDI driver itself.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-acx565akm.c | 13 +------------
drivers/video/omap2/dss/sdi.c | 17 +++++++++++++++++
include/video/omapdss.h | 2 ++
3 files changed, 20 insertions(+), 12 deletions(-)
@@ -146,6 +146,23 @@ void omapdss_sdi_display_disable(struct omap_dss_device *dssdev)}EXPORT_SYMBOL(omapdss_sdi_display_disable);+voidomapdss_sdi_set_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings)+{+intr;++dssdev->panel.timings=*timings;++if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){+omapdss_sdi_display_disable(dssdev);++r=omapdss_sdi_display_enable(dssdev);+if(r)+DSSERR("failed to set new timings\n");+}+}+EXPORT_SYMBOL(omapdss_sdi_set_timings);+staticint__initsdi_init_display(structomap_dss_device*dssdev){DSSDBG("SDI init\n");
The SDI driver currently relies on the timings in omap_dss_device struct to
configure the DISPC accordingly. This makes the SDI interface driver dependent
on the omap_dss_device struct.
Make the SDI driver data maintain it's own timings field. The panel driver is
expected to call omapdss_sdi_set_timings() to set these timings before the panel
is enabled.
Make the SDI panel driver configure the new timings is the omap_dss_device
struct(dssdev->panel.timings). The SDI driver is responsible for maintaining
only it's own copy of timings.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/displays/panel-acx565akm.c | 4 ++++
drivers/video/omap2/dss/sdi.c | 5 +++--
2 files changed, 7 insertions(+), 2 deletions(-)
The current venc.c driver contains both the interface and panel driver code.
This makes the driver hard to read, and difficult to understand the work split
between the interface and panel driver and the how the locking works.
This also makes it easier to clearly define the VENC interface ops called by the
panel driver.
Split venc.c into venc.c and venc_panel.c representing the interface and panel
driver respectively. This split is done along the lines of the HDMI interface
and panel drivers.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/Makefile | 2 +-
drivers/video/omap2/dss/dss.h | 10 ++
drivers/video/omap2/dss/venc.c | 208 ++++++++----------------------
drivers/video/omap2/dss/venc_panel.c | 231 ++++++++++++++++++++++++++++++++++
4 files changed, 295 insertions(+), 156 deletions(-)
create mode 100644 drivers/video/omap2/dss/venc_panel.c
@@ -491,171 +490,95 @@ unsigned long venc_get_pixel_clock(void)return13500000;}-staticssize_tdisplay_output_type_show(structdevice*dev,-structdevice_attribute*attr,char*buf)+intomapdss_venc_display_enable(structomap_dss_device*dssdev){-structomap_dss_device*dssdev=to_dss_device(dev);-constchar*ret;--switch(dssdev->phy.venc.type){-caseOMAP_DSS_VENC_TYPE_COMPOSITE:-ret="composite";-break;-caseOMAP_DSS_VENC_TYPE_SVIDEO:-ret="svideo";-break;-default:-return-EINVAL;-}+intr;-returnsnprintf(buf,PAGE_SIZE,"%s\n",ret);-}--staticssize_tdisplay_output_type_store(structdevice*dev,-structdevice_attribute*attr,constchar*buf,size_tsize)-{-structomap_dss_device*dssdev=to_dss_device(dev);-enumomap_dss_venc_typenew_type;--if(sysfs_streq("composite",buf))-new_type=OMAP_DSS_VENC_TYPE_COMPOSITE;-elseif(sysfs_streq("svideo",buf))-new_type=OMAP_DSS_VENC_TYPE_SVIDEO;-else-return-EINVAL;+DSSDBG("venc_display_enable\n");mutex_lock(&venc.venc_lock);-if(dssdev->phy.venc.type!=new_type){-dssdev->phy.venc.type=new_type;-if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){-venc_power_off(dssdev);-venc_power_on(dssdev);-}+if(dssdev->manager=NULL){+DSSERR("Failed to enable display: no manager\n");+r=-ENODEV;+gotoerr0;}-mutex_unlock(&venc.venc_lock);--returnsize;-}--staticDEVICE_ATTR(output_type,S_IRUGO|S_IWUSR,-display_output_type_show,display_output_type_store);--/* driver */-staticintvenc_panel_probe(structomap_dss_device*dssdev)-{-dssdev->panel.timings=omap_dss_pal_timings;--returndevice_create_file(&dssdev->dev,&dev_attr_output_type);-}--staticvoidvenc_panel_remove(structomap_dss_device*dssdev)-{-device_remove_file(&dssdev->dev,&dev_attr_output_type);-}--staticintvenc_panel_enable(structomap_dss_device*dssdev)-{-intr=0;--DSSDBG("venc_enable_display\n");--mutex_lock(&venc.venc_lock);-r=omap_dss_start_device(dssdev);if(r){DSSERR("failed to start device\n");gotoerr0;}-if(dssdev->state!=OMAP_DSS_DISPLAY_DISABLED){-r=-EINVAL;-gotoerr1;-}+if(dssdev->platform_enable)+dssdev->platform_enable(dssdev);-r=venc_runtime_get();-if(r)-gotoerr1;r=venc_power_on(dssdev);if(r)-gotoerr2;+gotoerr1;venc.wss_data=0;-dssdev->state=OMAP_DSS_DISPLAY_ACTIVE;-mutex_unlock(&venc.venc_lock);+return0;-err2:-venc_runtime_put();err1:+if(dssdev->platform_disable)+dssdev->platform_disable(dssdev);omap_dss_stop_device(dssdev);err0:mutex_unlock(&venc.venc_lock);-returnr;}-staticvoidvenc_panel_disable(structomap_dss_device*dssdev)+voidomapdss_venc_display_disable(structomap_dss_device*dssdev){-DSSDBG("venc_disable_display\n");+DSSDBG("venc_display_disable\n");mutex_lock(&venc.venc_lock);-if(dssdev->state=OMAP_DSS_DISPLAY_DISABLED)-gotoend;--if(dssdev->state=OMAP_DSS_DISPLAY_SUSPENDED){-/* suspended is the same as disabled with venc */-dssdev->state=OMAP_DSS_DISPLAY_DISABLED;-gotoend;-}-venc_power_off(dssdev);-venc_runtime_put();--dssdev->state=OMAP_DSS_DISPLAY_DISABLED;-omap_dss_stop_device(dssdev);-end:-mutex_unlock(&venc.venc_lock);-}-staticintvenc_panel_suspend(structomap_dss_device*dssdev)-{-venc_panel_disable(dssdev);-return0;-}+if(dssdev->platform_disable)+dssdev->platform_disable(dssdev);-staticintvenc_panel_resume(structomap_dss_device*dssdev)-{-returnvenc_panel_enable(dssdev);+mutex_unlock(&venc.venc_lock);}-staticvoidvenc_set_timings(structomap_dss_device*dssdev,-structomap_video_timings*timings)+voidomapdss_venc_set_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings){DSSDBG("venc_set_timings\n");+mutex_lock(&venc.venc_lock);+/* Reset WSS data when the TV standard changes. */if(memcmp(&dssdev->panel.timings,timings,sizeof(*timings)))venc.wss_data=0;dssdev->panel.timings=*timings;+if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){+intr;+/* turn the venc off and on to get new timings to use */-venc_panel_disable(dssdev);-venc_panel_enable(dssdev);+venc_power_off(dssdev);++r=venc_power_on(dssdev);+if(r)+DSSERR("failed to power on VENC\n");}else{dss_mgr_set_timings(dssdev->manager,timings);}++mutex_unlock(&venc.venc_lock);}-staticintvenc_check_timings(structomap_dss_device*dssdev,-structomap_video_timings*timings)+intomapdss_venc_check_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings){DSSDBG("venc_check_timings\n");
@@ -668,13 +591,13 @@ static int venc_check_timings(struct omap_dss_device *dssdev,return-EINVAL;}-staticu32venc_get_wss(structomap_dss_device*dssdev)+u32omapdss_venc_get_wss(structomap_dss_device*dssdev){/* Invert due to VENC_L21_WC_CTL:INV=1 */return(venc.wss_data>>8)^0xfffff;}-staticintvenc_set_wss(structomap_dss_device*dssdev,u32wss)+intomapdss_venc_set_wss(structomap_dss_device*dssdev,u32wss){conststructvenc_config*config;intr;
@@ -703,31 +626,6 @@ err:returnr;}-staticstructomap_dss_drivervenc_driver={-.probe=venc_panel_probe,-.remove=venc_panel_remove,--.enable=venc_panel_enable,-.disable=venc_panel_disable,-.suspend=venc_panel_suspend,-.resume=venc_panel_resume,--.get_resolution=omapdss_default_get_resolution,-.get_recommended_bpp=omapdss_default_get_recommended_bpp,--.set_timings=venc_set_timings,-.check_timings=venc_check_timings,--.get_wss=venc_get_wss,-.set_wss=venc_set_wss,--.driver={-.name="venc",-.owner=THIS_MODULE,-},-};-/* driver end */-staticint__initvenc_init_display(structomap_dss_device*dssdev){DSSDBG("init_display\n");
@@ -897,9 +795,9 @@ static int __init omap_venchw_probe(struct platform_device *pdev)venc_runtime_put();-r=omap_dss_register_driver(&venc_driver);+r=venc_panel_init();if(r)-gotoerr_reg_panel_driver;+gotoerr_panel_init;dss_debugfs_create_file("venc",venc_dump_regs);
@@ -907,7 +805,7 @@ static int __init omap_venchw_probe(struct platform_device *pdev)return0;-err_reg_panel_driver:+err_panel_init:err_runtime_get:pm_runtime_disable(&pdev->dev);venc_put_clocks();
@@ -923,7 +821,7 @@ static int __exit omap_venchw_remove(struct platform_device *pdev)venc.vdda_dac_reg=NULL;}-omap_dss_unregister_driver(&venc_driver);+venc_panel_exit();pm_runtime_disable(&pdev->dev);venc_put_clocks();
@@ -0,0 +1,231 @@+/*+*Copyright(C)2009NokiaCorporation+*Author:TomiValkeinen<tomi.valkeinen@nokia.com>+*+*VENCpaneldriver+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsoftheGNUGeneralPublicLicenseversion2aspublishedby+*theFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++#include<linux/kernel.h>+#include<linux/err.h>+#include<linux/io.h>+#include<linux/mutex.h>+#include<linux/module.h>++#include<video/omapdss.h>++#include"dss.h"++staticstruct{+structmutexlock;+}venc_panel;++staticssize_tdisplay_output_type_show(structdevice*dev,+structdevice_attribute*attr,char*buf)+{+structomap_dss_device*dssdev=to_dss_device(dev);+constchar*ret;++switch(dssdev->phy.venc.type){+caseOMAP_DSS_VENC_TYPE_COMPOSITE:+ret="composite";+break;+caseOMAP_DSS_VENC_TYPE_SVIDEO:+ret="svideo";+break;+default:+return-EINVAL;+}++returnsnprintf(buf,PAGE_SIZE,"%s\n",ret);+}++staticssize_tdisplay_output_type_store(structdevice*dev,+structdevice_attribute*attr,constchar*buf,size_tsize)+{+structomap_dss_device*dssdev=to_dss_device(dev);+enumomap_dss_venc_typenew_type;++if(sysfs_streq("composite",buf))+new_type=OMAP_DSS_VENC_TYPE_COMPOSITE;+elseif(sysfs_streq("svideo",buf))+new_type=OMAP_DSS_VENC_TYPE_SVIDEO;+else+return-EINVAL;++mutex_lock(&venc_panel.lock);++if(dssdev->phy.venc.type!=new_type){+dssdev->phy.venc.type=new_type;+if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){+omapdss_venc_display_disable(dssdev);+omapdss_venc_display_enable(dssdev);+}+}++mutex_unlock(&venc_panel.lock);++returnsize;+}++staticDEVICE_ATTR(output_type,S_IRUGO|S_IWUSR,+display_output_type_show,display_output_type_store);++staticintvenc_panel_probe(structomap_dss_device*dssdev)+{+mutex_init(&venc_panel.lock);++/* set initial timings to PAL */+dssdev->panel.timings=(structomap_video_timings)+{720,574,13500,64,12,68,5,5,41,+OMAPDSS_SIG_ACTIVE_HIGH,OMAPDSS_SIG_ACTIVE_HIGH,+true,+};++returndevice_create_file(&dssdev->dev,&dev_attr_output_type);+}++staticvoidvenc_panel_remove(structomap_dss_device*dssdev)+{+device_remove_file(&dssdev->dev,&dev_attr_output_type);+}++staticintvenc_panel_enable(structomap_dss_device*dssdev)+{+intr;++dev_dbg(&dssdev->dev,"venc_panel_enable\n");++mutex_lock(&venc_panel.lock);++if(dssdev->state!=OMAP_DSS_DISPLAY_DISABLED){+r=-EINVAL;+gotoerr;+}++r=omapdss_venc_display_enable(dssdev);+if(r)+gotoerr;++dssdev->state=OMAP_DSS_DISPLAY_ACTIVE;++mutex_unlock(&venc_panel.lock);++return0;+err:+mutex_unlock(&venc_panel.lock);++returnr;+}++staticvoidvenc_panel_disable(structomap_dss_device*dssdev)+{+dev_dbg(&dssdev->dev,"venc_panel_disable\n");++mutex_lock(&venc_panel.lock);++if(dssdev->state=OMAP_DSS_DISPLAY_DISABLED)+gotoend;++if(dssdev->state=OMAP_DSS_DISPLAY_SUSPENDED){+/* suspended is the same as disabled with venc */+dssdev->state=OMAP_DSS_DISPLAY_DISABLED;+gotoend;+}++omapdss_venc_display_disable(dssdev);++dssdev->state=OMAP_DSS_DISPLAY_DISABLED;+end:+mutex_unlock(&venc_panel.lock);+}++staticintvenc_panel_suspend(structomap_dss_device*dssdev)+{+venc_panel_disable(dssdev);+return0;+}++staticintvenc_panel_resume(structomap_dss_device*dssdev)+{+returnvenc_panel_enable(dssdev);+}++staticvoidvenc_panel_set_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings)+{+dev_dbg(&dssdev->dev,"venc_panel_set_timings\n");++mutex_lock(&venc_panel.lock);++omapdss_venc_set_timings(dssdev,timings);++mutex_unlock(&venc_panel.lock);+}++staticintvenc_panel_check_timings(structomap_dss_device*dssdev,+structomap_video_timings*timings)+{+dev_dbg(&dssdev->dev,"venc_panel_check_timings\n");++returnomapdss_venc_check_timings(dssdev,timings);+}++staticu32venc_panel_get_wss(structomap_dss_device*dssdev)+{+dev_dbg(&dssdev->dev,"venc_panel_get_wss\n");++returnomapdss_venc_get_wss(dssdev);+}++staticintvenc_panel_set_wss(structomap_dss_device*dssdev,u32wss)+{+dev_dbg(&dssdev->dev,"venc_panel_set_wss\n");++returnomapdss_venc_set_wss(dssdev,wss);+}++staticstructomap_dss_drivervenc_driver={+.probe=venc_panel_probe,+.remove=venc_panel_remove,++.enable=venc_panel_enable,+.disable=venc_panel_disable,+.suspend=venc_panel_suspend,+.resume=venc_panel_resume,++.get_resolution=omapdss_default_get_resolution,+.get_recommended_bpp=omapdss_default_get_recommended_bpp,++.set_timings=venc_panel_set_timings,+.check_timings=venc_panel_check_timings,++.get_wss=venc_panel_get_wss,+.set_wss=venc_panel_set_wss,++.driver={+.name="venc",+.owner=THIS_MODULE,+},+};++intvenc_panel_init(void)+{+returnomap_dss_register_driver(&venc_driver);+}++voidvenc_panel_exit(void)+{+omap_dss_unregister_driver(&venc_driver);+}
The VENC driver currently relies on the timings in omap_dss_device struct to
configure the DISPC and VENC blocks accordingly. This makes the VENC interface
driver dependent on the omap_dss_device struct.
Make the VENC driver data maintain it's own timings field. The panel driver is
expected to call omapdss_venc_set_timings() to set these timings before the
panel is enabled.
Make the VENC panel driver configure the new timings is the omap_dss_device
struct(dssdev->panel.timings). The VENC driver is responsible for maintaining
only it's own copy of timings.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/venc.c | 12 +++++++-----
drivers/video/omap2/dss/venc_panel.c | 1 +
2 files changed, 8 insertions(+), 5 deletions(-)
@@ -432,7 +434,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)gotoerr0;venc_reset();-venc_write_config(venc_timings_to_config(&dssdev->panel.timings));+venc_write_config(venc_timings_to_config(&venc.timings));dss_set_venc_output(dssdev->phy.venc.type);dss_set_dac_pwrdn_bgz(1);
@@ -449,7 +451,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)venc_write_reg(VENC_OUTPUT_CONTROL,l);-dss_mgr_set_timings(dssdev->manager,&dssdev->panel.timings);+dss_mgr_set_timings(dssdev->manager,&venc.timings);r=regulator_enable(venc.vdda_dac_reg);if(r)
@@ -556,10 +558,10 @@ void omapdss_venc_set_timings(struct omap_dss_device *dssdev,mutex_lock(&venc.venc_lock);/* Reset WSS data when the TV standard changes. */-if(memcmp(&dssdev->panel.timings,timings,sizeof(*timings)))+if(memcmp(&venc.timings,timings,sizeof(*timings)))venc.wss_data=0;-dssdev->panel.timings=*timings;+venc.timings=*timings;if(dssdev->state=OMAP_DSS_DISPLAY_ACTIVE){intr;
@@ -606,7 +608,7 @@ int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss)mutex_lock(&venc.venc_lock);-config=venc_timings_to_config(&dssdev->panel.timings);+config=venc_timings_to_config(&venc.timings);/* Invert due to VENC_L21_WC_CTL:INV=1 */venc.wss_data=(wss^0xfffff)<<8;
Add function omapdss_venc_get_timing() which returns the timings
maintained by the VENC interface driver in it's driver data. This is just used
once by the driver during it's probe. This prevents the need for the panel
driver to configure default timings in it's probe.
Int the VENC interface's probe, the timings field is set to PAL as a default
value. The get_timing op makes more sense for interfaces which can be configured
to a default timing.
Signed-off-by: Archit Taneja <redacted>
---
drivers/video/omap2/dss/dss.h | 2 ++
drivers/video/omap2/dss/venc.c | 13 +++++++++++++
drivers/video/omap2/dss/venc_panel.c | 11 +++++------
3 files changed, 20 insertions(+), 6 deletions(-)
@@ -84,14 +84,13 @@ static DEVICE_ATTR(output_type, S_IRUGO | S_IWUSR,staticintvenc_panel_probe(structomap_dss_device*dssdev){+structomap_video_timingstimings;+mutex_init(&venc_panel.lock);-/* set initial timings to PAL */-dssdev->panel.timings=(structomap_video_timings)-{720,574,13500,64,12,68,5,5,41,-OMAPDSS_SIG_ACTIVE_HIGH,OMAPDSS_SIG_ACTIVE_HIGH,-true,-};+omapdss_venc_get_timings(dssdev,&timings);++dssdev->panel.timings=timings;returndevice_create_file(&dssdev->dev,&dev_attr_output_type);}
From: Tomi Valkeinen <hidden> Date: 2012-08-14 13:02:28
Hi,
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
Add function omapdss_hdmi_display_get_timing() which returns the timings
maintained by the HDMI interface driver in it's hdmi_config field. This
prevents the need for the panel driver to configure default timings in it's
probe.
This function is just intended to be used once during the panel driver's probe.
It makes sense for those interfaces which can be configured to a default timing.
I'm not sure about this patch. So I think the basic idea here is that
HDMI will use VGA video mode if, for whatever reason, no better mode is
found out.
After this change, the panel driver doesn't seem to do that. Instead it
uses whatever mode was used previously by the hdmi output driver.
Is the only reason for this patch to clean up the hdmi_panel driver? We
could have a dss helper function which returns the timings for VGA
(well, just a public const variable would be enough), and the panel
driver could use that to initialize the timings struct.
Tomi
On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
Hi,
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
quoted
Add function omapdss_hdmi_display_get_timing() which returns the timings
maintained by the HDMI interface driver in it's hdmi_config field. This
prevents the need for the panel driver to configure default timings in it's
probe.
This function is just intended to be used once during the panel driver's probe.
It makes sense for those interfaces which can be configured to a default timing.
I'm not sure about this patch. So I think the basic idea here is that
HDMI will use VGA video mode if, for whatever reason, no better mode is
found out.
After this change, the panel driver doesn't seem to do that. Instead it
uses whatever mode was used previously by the hdmi output driver.
Is the only reason for this patch to clean up the hdmi_panel driver? We
could have a dss helper function which returns the timings for VGA
(well, just a public const variable would be enough), and the panel
driver could use that to initialize the timings struct.
Yes, that's the only reason, basically, to sync the panel with the
timings to which the hdmi output driver configured itself during it's
probe. We don't use hdmi_get_timing anywhere apart from here.
I thought it would be clean to retrieve the timings by taking whatever
is stored in the hdmi output driver, but we could leave it to a
hardcoded VGA as before.
I have done the same for venc, let me know if you think these should be
removed.
Archit
From: Tomi Valkeinen <hidden> Date: 2012-08-14 13:44:05
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
Create function omapdss_sdi_set_timings(). Configuring new timings is done the
same way as before, SDI is disabled, and re-enabled with the new timings in
dssdev. This just moves the code from the panel drivers to the SDI driver.
The panel drivers shouldn't be aware of how SDI manages to configure a new set
of timings. This should be taken care of by the SDI driver itself.
I'm not sure about this one. Although I see that dpi.c does currently
the same thing as you're doing in your patch.
One thing is that we should try to remove dssdev uses from the output
drivers, including use of dssdev->state.
The other thing is that I don't think the output driver should disable &
enable the output during set timings. I think sdi's set_timings should
return EBUSY if the output is enabled. The same way as other
configuration functions should (like dpi_set_data_lines or such).
I'm actually not sure if even the panel driver should disable & enable
the output during set_timings. Perhaps it should be the caller's
(omapdrm or such) responsibility....
My reasoning here is that disabling & enabling the video output is not
invisible to the upper layers, so doing it "in secret" may be bad.
Then again, perhaps timings can be changed freely on some other
platforms, and then it'd be nice if the panel driver wouldn't disable &
enable the output.
So I'm again not quite sure what's the best way to handle this... (of
the dssdev->state I'm sure, its use should be removed from omapdss). Any
thoughts?
Tomi
From: Tomi Valkeinen <hidden> Date: 2012-08-14 14:10:20
On Tue, 2012-08-14 at 18:45 +0530, Archit Taneja wrote:
On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
quoted
Hi,
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
quoted
Add function omapdss_hdmi_display_get_timing() which returns the timings
maintained by the HDMI interface driver in it's hdmi_config field. This
prevents the need for the panel driver to configure default timings in it's
probe.
This function is just intended to be used once during the panel driver's probe.
It makes sense for those interfaces which can be configured to a default timing.
I'm not sure about this patch. So I think the basic idea here is that
HDMI will use VGA video mode if, for whatever reason, no better mode is
found out.
After this change, the panel driver doesn't seem to do that. Instead it
uses whatever mode was used previously by the hdmi output driver.
Is the only reason for this patch to clean up the hdmi_panel driver? We
could have a dss helper function which returns the timings for VGA
(well, just a public const variable would be enough), and the panel
driver could use that to initialize the timings struct.
Yes, that's the only reason, basically, to sync the panel with the
timings to which the hdmi output driver configured itself during it's
probe. We don't use hdmi_get_timing anywhere apart from here.
Does the hdmi output driver even need to configure any defaults?
Wouldn't it be ok to presume that the panel driver configures the
timings?
I don't think it's sensible to think about default values in the output
driver generally (well, at least for things like timings). Although in
HDMI's case VGA is quite sensible default, but that's not the case for
any other output.
So I think generally we should just trust the panel driver to tell the
output driver what configuration should be used before the panel driver
enables the output.
That said, it doesn't hurt that the output drivers initialize their own
datastructures to something relatively sane to avoid any BUG() or WARN()
calls in the omapdss. Although we could also somehow track if the
timings has been set, and return an error when enabling the display if
timings hasn't been set. But that requires an extra flag, so perhaps
it's simpler to have some initial values in the output driver also.
I thought it would be clean to retrieve the timings by taking whatever
is stored in the hdmi output driver, but we could leave it to a
hardcoded VGA as before.
My main worry is that it's not easily clear what is going on if you look
at the panel code. It looks that the panel just uses whatever is in the
output driver. It's not clear that it is always VGA. What if the
previous mode was something else?
I think it's much clearer if the panel driver sets the timings
explicitly before enabling the output. It doesn't have to be in panel's
probe, although that's perhaps the easiest place for it.
I have done the same for venc, let me know if you think these should be
removed.
Well, I agree that the initial timings code in hdmi and venc is a bit
ugly. I don't think it's bad as such, just that the timings are standard
ones, and instead of having all the timings numbers there, we should
have a common place for them.
Tomi
On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
quoted
Create function omapdss_sdi_set_timings(). Configuring new timings is done the
same way as before, SDI is disabled, and re-enabled with the new timings in
dssdev. This just moves the code from the panel drivers to the SDI driver.
The panel drivers shouldn't be aware of how SDI manages to configure a new set
of timings. This should be taken care of by the SDI driver itself.
I'm not sure about this one. Although I see that dpi.c does currently
the same thing as you're doing in your patch.
Even HDMI does the same thing.
One thing is that we should try to remove dssdev uses from the output
drivers, including use of dssdev->state.
Yes, we could do that by keeping a state of the output(and also checking
state of the manager)
The other thing is that I don't think the output driver should disable &
enable the output during set timings. I think sdi's set_timings should
return EBUSY if the output is enabled. The same way as other
configuration functions should (like dpi_set_data_lines or such).
I'm actually not sure if even the panel driver should disable & enable
the output during set_timings. Perhaps it should be the caller's
(omapdrm or such) responsibility....
My reasoning here is that disabling & enabling the video output is not
invisible to the upper layers, so doing it "in secret" may be bad.
Then again, perhaps timings can be changed freely on some other
platforms, and then it'd be nice if the panel driver wouldn't disable &
enable the output.
So I'm again not quite sure what's the best way to handle this... (of
the dssdev->state I'm sure, its use should be removed from omapdss). Any
thoughts?
I guess it depends on how drm/fb want to use it. I guess an output
should have a set_timings() kind of op if it can do it seamlessly. I
guess we can do that easily in DPI, for example, we could reduce the fps
from 60 to 30 without causing an artefacts(I think). For outputs which
can't do it, we could remove the set_timings totally.
However, it'll be kind of inconsistent for some outputs to set timings,
and for others to not, and if in the future drm/fb gets exposed to ops
too, we may have dirty checks to see if set_timings is populated or not.
The easiest way would be to make all set_timings just update the copy of
the timings output has, and expect drm/fb to disable and re enable the
panel. We may end up doing unnecessary gpio resets and configuration of
the panels though.
Archit
On Tuesday 14 August 2012 07:40 PM, Tomi Valkeinen wrote:
On Tue, 2012-08-14 at 18:45 +0530, Archit Taneja wrote:
quoted
On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
quoted
Hi,
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
quoted
Add function omapdss_hdmi_display_get_timing() which returns the timings
maintained by the HDMI interface driver in it's hdmi_config field. This
prevents the need for the panel driver to configure default timings in it's
probe.
This function is just intended to be used once during the panel driver's probe.
It makes sense for those interfaces which can be configured to a default timing.
I'm not sure about this patch. So I think the basic idea here is that
HDMI will use VGA video mode if, for whatever reason, no better mode is
found out.
After this change, the panel driver doesn't seem to do that. Instead it
uses whatever mode was used previously by the hdmi output driver.
Is the only reason for this patch to clean up the hdmi_panel driver? We
could have a dss helper function which returns the timings for VGA
(well, just a public const variable would be enough), and the panel
driver could use that to initialize the timings struct.
Yes, that's the only reason, basically, to sync the panel with the
timings to which the hdmi output driver configured itself during it's
probe. We don't use hdmi_get_timing anywhere apart from here.
Does the hdmi output driver even need to configure any defaults?
Wouldn't it be ok to presume that the panel driver configures the
timings?
I don't think it's sensible to think about default values in the output
driver generally (well, at least for things like timings). Although in
HDMI's case VGA is quite sensible default, but that's not the case for
any other output.
So I think generally we should just trust the panel driver to tell the
output driver what configuration should be used before the panel driver
enables the output.
That said, it doesn't hurt that the output drivers initialize their own
datastructures to something relatively sane to avoid any BUG() or WARN()
calls in the omapdss. Although we could also somehow track if the
timings has been set, and return an error when enabling the display if
timings hasn't been set. But that requires an extra flag, so perhaps
it's simpler to have some initial values in the output driver also.
quoted
I thought it would be clean to retrieve the timings by taking whatever
is stored in the hdmi output driver, but we could leave it to a
hardcoded VGA as before.
My main worry is that it's not easily clear what is going on if you look
at the panel code. It looks that the panel just uses whatever is in the
output driver. It's not clear that it is always VGA. What if the
previous mode was something else?
I think it's much clearer if the panel driver sets the timings
explicitly before enabling the output. It doesn't have to be in panel's
probe, although that's perhaps the easiest place for it.
quoted
I have done the same for venc, let me know if you think these should be
removed.
Well, I agree that the initial timings code in hdmi and venc is a bit
ugly. I don't think it's bad as such, just that the timings are standard
ones, and instead of having all the timings numbers there, we should
have a common place for them.
Okay, I'll remove the get_timing ops, keep defaults in the panel
driver's probe, have a const variable for the defaults to make it look
less ugly, and make sure that there is a set_timings for the output in
the hdmi and venc panel drivers before they are enabled, it sort of okay
for hdmi and venc panel drivers as there is going to be only one of them
and be in our control.
Archit
From: Tomi Valkeinen <hidden> Date: 2012-08-14 17:33:20
On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
I guess it depends on how drm/fb want to use it. I guess an output
should have a set_timings() kind of op if it can do it seamlessly. I
guess we can do that easily in DPI, for example, we could reduce the fps
from 60 to 30 without causing an artefacts(I think). For outputs which
Yes, that kind of thing is easy to do by just changing the pck divider,
which is in a shadow register. I mean, "easy" in theory, at least. Our
clock calculation doesn't work like that currently, though, so it could
end up changing DSS fck.
can't do it, we could remove the set_timings totally.
But we do need set_timings for other outputs also (like sdi). We just
can't change them just like that. Only outputs that do not need timings
are DSI command mode and rfbi.
However, it'll be kind of inconsistent for some outputs to set timings,
and for others to not, and if in the future drm/fb gets exposed to ops
too, we may have dirty checks to see if set_timings is populated or not.
The easiest way would be to make all set_timings just update the copy of
the timings output has, and expect drm/fb to disable and re enable the
panel. We may end up doing unnecessary gpio resets and configuration of
the panels though.
I think changing things like timings is quite a rare operation. The only
case it'd be necessary to change timings often, with speed, and without
artifacts would be the fps drop you mentioned, for lower power use with
panels that don't mind the fps drop.
If I understood correctly, Rob said that drm already disables the output
when changing the mode, when I asked if it's ok for the apply's
set_timings to require the output to be off.
In any case this is not a big issue, I mean, it's not causing any
problems. Somebody is going to disable the output anyway when changing
the timings. Perhaps even these patches are good, because they make the
set_timings consistent across the output drivers (don't they?).
Tomi
On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote:
On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
quoted
On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
quoted
I guess it depends on how drm/fb want to use it. I guess an output
should have a set_timings() kind of op if it can do it seamlessly. I
guess we can do that easily in DPI, for example, we could reduce the fps
from 60 to 30 without causing an artefacts(I think). For outputs which
Yes, that kind of thing is easy to do by just changing the pck divider,
which is in a shadow register. I mean, "easy" in theory, at least. Our
clock calculation doesn't work like that currently, though, so it could
end up changing DSS fck.
quoted
can't do it, we could remove the set_timings totally.
But we do need set_timings for other outputs also (like sdi). We just
can't change them just like that. Only outputs that do not need timings
are DSI command mode and rfbi.
quoted
However, it'll be kind of inconsistent for some outputs to set timings,
and for others to not, and if in the future drm/fb gets exposed to ops
too, we may have dirty checks to see if set_timings is populated or not.
The easiest way would be to make all set_timings just update the copy of
the timings output has, and expect drm/fb to disable and re enable the
panel. We may end up doing unnecessary gpio resets and configuration of
the panels though.
I think changing things like timings is quite a rare operation. The only
case it'd be necessary to change timings often, with speed, and without
artifacts would be the fps drop you mentioned, for lower power use with
panels that don't mind the fps drop.
If I understood correctly, Rob said that drm already disables the output
when changing the mode, when I asked if it's ok for the apply's
set_timings to require the output to be off.
In any case this is not a big issue, I mean, it's not causing any
problems. Somebody is going to disable the output anyway when changing
the timings. Perhaps even these patches are good, because they make the
set_timings consistent across the output drivers (don't they?).
Yes, they do, there isn't a set_timings for RFBI though, only a
set_size, and DSI has set_timings for video mode and a set_size for
command mode, I haven't put checks in the ops for a panel driver to
wrongly call set_timings in commmand mode, and set_size in video mode.
I haven't done that yet because a future patch of mine will have a DSI
specific op called set_operation_mode() to make us independent of
dssdev->panel.dsi_mode, there is no guarantee that the panel driver to
first call set_operation_mode(), and then set_timings(), so I'm not sure
yet how to deal with that. Probably having a mode/state which says that
a field is unintialized might help, but that would overcomplicate things.
Archit
From: Rob Clark <hidden> Date: 2012-08-14 19:26:06
On Tue, Aug 14, 2012 at 11:56 AM, Archit Taneja [off-list ref] wrote:
On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
quoted
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
quoted
Create function omapdss_sdi_set_timings(). Configuring new timings is
done the
same way as before, SDI is disabled, and re-enabled with the new timings
in
dssdev. This just moves the code from the panel drivers to the SDI
driver.
The panel drivers shouldn't be aware of how SDI manages to configure a
new set
of timings. This should be taken care of by the SDI driver itself.
I'm not sure about this one. Although I see that dpi.c does currently
the same thing as you're doing in your patch.
Even HDMI does the same thing.
quoted
One thing is that we should try to remove dssdev uses from the output
drivers, including use of dssdev->state.
Yes, we could do that by keeping a state of the output(and also checking
state of the manager)
quoted
The other thing is that I don't think the output driver should disable &
enable the output during set timings. I think sdi's set_timings should
return EBUSY if the output is enabled. The same way as other
configuration functions should (like dpi_set_data_lines or such).
I'm actually not sure if even the panel driver should disable & enable
the output during set_timings. Perhaps it should be the caller's
(omapdrm or such) responsibility....
My reasoning here is that disabling & enabling the video output is not
invisible to the upper layers, so doing it "in secret" may be bad.
Then again, perhaps timings can be changed freely on some other
platforms, and then it'd be nice if the panel driver wouldn't disable &
enable the output.
So I'm again not quite sure what's the best way to handle this... (of
the dssdev->state I'm sure, its use should be removed from omapdss). Any
thoughts?
I guess it depends on how drm/fb want to use it. I guess an output should
have a set_timings() kind of op if it can do it seamlessly. I guess we can
do that easily in DPI, for example, we could reduce the fps from 60 to 30
without causing an artefacts(I think). For outputs which can't do it, we
could remove the set_timings totally.
fwiw, drm wouldn't try to change timings on the fly.. or at least it
is bracketed by a call to the driver's crtc->prepare() and
crtc->commit() fxns (which in our case disable/enable output).
I haven't seen much userspace that tries to do things like this,
except maybe some apps like xbmc which seem to have some options to
attempt to change timings to align w/ video playback framerate. I am
a bit curious how many tv's and drivers could support this in a
glitch-free way.
BR,
-R
However, it'll be kind of inconsistent for some outputs to set timings, and
for others to not, and if in the future drm/fb gets exposed to ops too, we
may have dirty checks to see if set_timings is populated or not.
The easiest way would be to make all set_timings just update the copy of the
timings output has, and expect drm/fb to disable and re enable the panel. We
may end up doing unnecessary gpio resets and configuration of the panels
though.
Archit
--
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
From: Tomi Valkeinen <hidden> Date: 2012-08-15 06:43:26
On Wed, 2012-08-15 at 00:38 +0530, Archit Taneja wrote:
On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote:
quoted
On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
quoted
On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
quoted
I guess it depends on how drm/fb want to use it. I guess an output
should have a set_timings() kind of op if it can do it seamlessly. I
guess we can do that easily in DPI, for example, we could reduce the fps
from 60 to 30 without causing an artefacts(I think). For outputs which
Yes, that kind of thing is easy to do by just changing the pck divider,
which is in a shadow register. I mean, "easy" in theory, at least. Our
clock calculation doesn't work like that currently, though, so it could
end up changing DSS fck.
quoted
can't do it, we could remove the set_timings totally.
But we do need set_timings for other outputs also (like sdi). We just
can't change them just like that. Only outputs that do not need timings
are DSI command mode and rfbi.
quoted
However, it'll be kind of inconsistent for some outputs to set timings,
and for others to not, and if in the future drm/fb gets exposed to ops
too, we may have dirty checks to see if set_timings is populated or not.
The easiest way would be to make all set_timings just update the copy of
the timings output has, and expect drm/fb to disable and re enable the
panel. We may end up doing unnecessary gpio resets and configuration of
the panels though.
I think changing things like timings is quite a rare operation. The only
case it'd be necessary to change timings often, with speed, and without
artifacts would be the fps drop you mentioned, for lower power use with
panels that don't mind the fps drop.
If I understood correctly, Rob said that drm already disables the output
when changing the mode, when I asked if it's ok for the apply's
set_timings to require the output to be off.
In any case this is not a big issue, I mean, it's not causing any
problems. Somebody is going to disable the output anyway when changing
the timings. Perhaps even these patches are good, because they make the
set_timings consistent across the output drivers (don't they?).
Yes, they do, there isn't a set_timings for RFBI though, only a
set_size, and DSI has set_timings for video mode and a set_size for
command mode, I haven't put checks in the ops for a panel driver to
wrongly call set_timings in commmand mode, and set_size in video mode.
Ok. Well, perhaps we should go forward with these patches then. They
make things consistent, and we don't really know which would be the best
way to handle this, so the method in your patches is as good as some
other.
The dssdev->state needs to be removed at some point, but that can be a
separate task.
I haven't done that yet because a future patch of mine will have a DSI
specific op called set_operation_mode() to make us independent of
dssdev->panel.dsi_mode, there is no guarantee that the panel driver to
first call set_operation_mode(), and then set_timings(), so I'm not sure
yet how to deal with that. Probably having a mode/state which says that
a field is unintialized might help, but that would overcomplicate things.
Well, we can define that set_operation_mode needs to be called before
any other dsi functions. They are kernel drivers, we can presume they
act correctly (although we should of course try to handle error cases
anyway). If they don't, we need to fix them.
If you want to be extra safe there, you could add third value to the
dsi_mode enum: UNDEFINED or such (I guess this is what you meant also).
By default dsi's mode would be undefined, and functions could check it.
But I agree it'd add lots of checks all around.
Tomi