Re: [PATCH v3 3/8] drm/omap: Make fixed resolution panels work

2013-03-27 Thread Tomi Valkeinen
On 2013-03-26 15:45, Archit Taneja wrote:
 The omapdrm driver requires omapdss panel drivers to expose ops like detect,
 set_timings and check_timings. These can be NULL for fixed panel DPI, DBI, DSI
 and SDI drivers. At some places, there are no checks to see if the panel 
 driver
 has these ops or not, and that leads to a crash.
 
 The following things are done to make fixed panels work:
 
 - The omap_connector's detect function is modified such that it considers 
 panel
   types which are generally fixed panels as always connected(provided the 
 panel
   driver doesn't have a detect op). Hence, the connector corresponding to 
 these
   panels is always in a 'connected' state.
 
 - If a panel driver doesn't have a check_timings op, assume that it supports 
 the
   mode passed to omap_connector_mode_valid(the 'mode_valid' drm helper 
 function)
 
 - The function omap_encoder_update shouldn't really do anything for fixed
   resolution panels, make sure that it calls set_timings only if the panel
   driver has one.
 
 Signed-off-by: Archit Taneja arc...@ti.com
 ---
 v3: clear the timings local variable first before using memcmp
 v2: make sure the timings we try to set for a fixed resolution panel match the
 panel's timings
 
  drivers/gpu/drm/omapdrm/omap_connector.c |   27 +--
  drivers/gpu/drm/omapdrm/omap_encoder.c   |   17 +++--
  2 files changed, 40 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c 
 b/drivers/gpu/drm/omapdrm/omap_connector.c
 index c451c41..912759d 100644
 --- a/drivers/gpu/drm/omapdrm/omap_connector.c
 +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
 @@ -110,6 +110,11 @@ static enum drm_connector_status omap_connector_detect(
   ret = connector_status_connected;
   else
   ret = connector_status_disconnected;
 + } else if (dssdev-type == OMAP_DISPLAY_TYPE_DPI ||
 + dssdev-type == OMAP_DISPLAY_TYPE_DBI ||
 + dssdev-type == OMAP_DISPLAY_TYPE_SDI ||
 + dssdev-type == OMAP_DISPLAY_TYPE_DSI) {
 + ret = connector_status_connected;
   } else {
   ret = connector_status_unknown;
   }

Can we leave this part out? I don't like hardcoding things like that,
and if I'm not mistaken, this only affects VENC.

If the code above would use detect where available, and presume the
panel is connected in other cases, it'll be right for all other displays
but VENC, for which we have no connect information.

Or, there could be a special case for VENC, like above, which sets the
connector status to unknown for that. And connected for all others. It'd
still be hardcoded, but for much less cases.

 Tomi




signature.asc
Description: OpenPGP digital signature


Re: [PATCH v3 3/8] drm/omap: Make fixed resolution panels work

2013-03-27 Thread Archit Taneja

On Wednesday 27 March 2013 12:54 PM, Tomi Valkeinen wrote:

On 2013-03-26 15:45, Archit Taneja wrote:

The omapdrm driver requires omapdss panel drivers to expose ops like detect,
set_timings and check_timings. These can be NULL for fixed panel DPI, DBI, DSI
and SDI drivers. At some places, there are no checks to see if the panel driver
has these ops or not, and that leads to a crash.

The following things are done to make fixed panels work:

- The omap_connector's detect function is modified such that it considers panel
   types which are generally fixed panels as always connected(provided the panel
   driver doesn't have a detect op). Hence, the connector corresponding to these
   panels is always in a 'connected' state.

- If a panel driver doesn't have a check_timings op, assume that it supports the
   mode passed to omap_connector_mode_valid(the 'mode_valid' drm helper 
function)

- The function omap_encoder_update shouldn't really do anything for fixed
   resolution panels, make sure that it calls set_timings only if the panel
   driver has one.

Signed-off-by: Archit Taneja arc...@ti.com
---
v3: clear the timings local variable first before using memcmp
v2: make sure the timings we try to set for a fixed resolution panel match the
 panel's timings

  drivers/gpu/drm/omapdrm/omap_connector.c |   27 +--
  drivers/gpu/drm/omapdrm/omap_encoder.c   |   17 +++--
  2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c 
b/drivers/gpu/drm/omapdrm/omap_connector.c
index c451c41..912759d 100644
--- a/drivers/gpu/drm/omapdrm/omap_connector.c
+++ b/drivers/gpu/drm/omapdrm/omap_connector.c
@@ -110,6 +110,11 @@ static enum drm_connector_status omap_connector_detect(
ret = connector_status_connected;
else
ret = connector_status_disconnected;
+   } else if (dssdev-type == OMAP_DISPLAY_TYPE_DPI ||
+   dssdev-type == OMAP_DISPLAY_TYPE_DBI ||
+   dssdev-type == OMAP_DISPLAY_TYPE_SDI ||
+   dssdev-type == OMAP_DISPLAY_TYPE_DSI) {
+   ret = connector_status_connected;
} else {
ret = connector_status_unknown;
}


Can we leave this part out? I don't like hardcoding things like that,
and if I'm not mistaken, this only affects VENC.

If the code above would use detect where available, and presume the
panel is connected in other cases, it'll be right for all other displays
but VENC, for which we have no connect information.

Or, there could be a special case for VENC, like above, which sets the
connector status to unknown for that. And connected for all others. It'd
still be hardcoded, but for much less cases.


I guess we can leave the status for VENC as always connected too, if we 
leave it as unknown by default and have no detect func for VENC, it 
would never run with omapdrm.


Archit

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v3 3/8] drm/omap: Make fixed resolution panels work

2013-03-26 Thread Archit Taneja
The omapdrm driver requires omapdss panel drivers to expose ops like detect,
set_timings and check_timings. These can be NULL for fixed panel DPI, DBI, DSI
and SDI drivers. At some places, there are no checks to see if the panel driver
has these ops or not, and that leads to a crash.

The following things are done to make fixed panels work:

- The omap_connector's detect function is modified such that it considers panel
  types which are generally fixed panels as always connected(provided the panel
  driver doesn't have a detect op). Hence, the connector corresponding to these
  panels is always in a 'connected' state.

- If a panel driver doesn't have a check_timings op, assume that it supports the
  mode passed to omap_connector_mode_valid(the 'mode_valid' drm helper function)

- The function omap_encoder_update shouldn't really do anything for fixed
  resolution panels, make sure that it calls set_timings only if the panel
  driver has one.

Signed-off-by: Archit Taneja arc...@ti.com
---
v3: clear the timings local variable first before using memcmp
v2: make sure the timings we try to set for a fixed resolution panel match the
panel's timings

 drivers/gpu/drm/omapdrm/omap_connector.c |   27 +--
 drivers/gpu/drm/omapdrm/omap_encoder.c   |   17 +++--
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c 
b/drivers/gpu/drm/omapdrm/omap_connector.c
index c451c41..912759d 100644
--- a/drivers/gpu/drm/omapdrm/omap_connector.c
+++ b/drivers/gpu/drm/omapdrm/omap_connector.c
@@ -110,6 +110,11 @@ static enum drm_connector_status omap_connector_detect(
ret = connector_status_connected;
else
ret = connector_status_disconnected;
+   } else if (dssdev-type == OMAP_DISPLAY_TYPE_DPI ||
+   dssdev-type == OMAP_DISPLAY_TYPE_DBI ||
+   dssdev-type == OMAP_DISPLAY_TYPE_SDI ||
+   dssdev-type == OMAP_DISPLAY_TYPE_DSI) {
+   ret = connector_status_connected;
} else {
ret = connector_status_unknown;
}
@@ -189,12 +194,30 @@ static int omap_connector_mode_valid(struct drm_connector 
*connector,
struct omap_video_timings timings = {0};
struct drm_device *dev = connector-dev;
struct drm_display_mode *new_mode;
-   int ret = MODE_BAD;
+   int r, ret = MODE_BAD;
 
copy_timings_drm_to_omap(timings, mode);
mode-vrefresh = drm_mode_vrefresh(mode);
 
-   if (!dssdrv-check_timings(dssdev, timings)) {
+   /*
+* if the panel driver doesn't have a check_timings, it's most likely
+* a fixed resolution panel, check if the timings match with the
+* panel's timings
+*/
+   if (dssdrv-check_timings) {
+   r = dssdrv-check_timings(dssdev, timings);
+   } else {
+   struct omap_video_timings t = {0};
+
+   dssdrv-get_timings(dssdev, t);
+
+   if (memcmp(timings, t, sizeof(struct omap_video_timings)))
+   r = -EINVAL;
+   else
+   r = 0;
+   }
+
+   if (!r) {
/* check if vrefresh is still valid */
new_mode = drm_mode_duplicate(dev, mode);
new_mode-clock = timings.pixel_clock;
diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c 
b/drivers/gpu/drm/omapdrm/omap_encoder.c
index d48df71..c29451b 100644
--- a/drivers/gpu/drm/omapdrm/omap_encoder.c
+++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
@@ -135,13 +135,26 @@ int omap_encoder_update(struct drm_encoder *encoder,
 
dssdev-output-manager = mgr;
 
-   ret = dssdrv-check_timings(dssdev, timings);
+   if (dssdrv-check_timings) {
+   ret = dssdrv-check_timings(dssdev, timings);
+   } else {
+   struct omap_video_timings t = {0};
+
+   dssdrv-get_timings(dssdev, t);
+
+   if (memcmp(timings, t, sizeof(struct omap_video_timings)))
+   ret = -EINVAL;
+   else
+   ret = 0;
+   }
+
if (ret) {
dev_err(dev-dev, could not set timings: %d\n, ret);
return ret;
}
 
-   dssdrv-set_timings(dssdev, timings);
+   if (dssdrv-set_timings)
+   dssdrv-set_timings(dssdev, timings);
 
return 0;
 }
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html