* [PATCH] omapdrm: Make fixed resolution panels work
@ 2013-02-14 11:52 Archit Taneja
2013-02-14 15:54 ` Rob Clark
0 siblings, 1 reply; 5+ messages in thread
From: Archit Taneja @ 2013-02-14 11:52 UTC (permalink / raw)
To: robdclark; +Cc: tomi.valkeinen, linux-omap, dri-devel, greg, 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.
I tested this with picodlp on a OMAP4 sdp board. I couldn't get any other
working boards with fixed DPI panels. I could try the DSI video mode panel
on an OMAP5 sevm, but that will require me to patch a lot of things to get
omap5 dss work with DT. I can try if needed.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/staging/omapdrm/omap_connector.c | 10 ++++++++--
drivers/staging/omapdrm/omap_encoder.c | 6 ++++--
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/omapdrm/omap_connector.c b/drivers/staging/omapdrm/omap_connector.c
index 4cc9ee7..839c6e4 100644
--- a/drivers/staging/omapdrm/omap_connector.c
+++ b/drivers/staging/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,13 @@ 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)) {
+ r = dssdrv->check_timings ? dssdrv->check_timings(dssdev, &timings) : 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/staging/omapdrm/omap_encoder.c b/drivers/staging/omapdrm/omap_encoder.c
index e053160..871af12e 100644
--- a/drivers/staging/omapdrm/omap_encoder.c
+++ b/drivers/staging/omapdrm/omap_encoder.c
@@ -128,13 +128,15 @@ int omap_encoder_update(struct drm_encoder *encoder,
dssdev->output->manager = mgr;
- ret = dssdrv->check_timings(dssdev, timings);
+ ret = dssdrv->check_timings ?
+ dssdrv->check_timings(dssdev, timings) : 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.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] omapdrm: Make fixed resolution panels work
2013-02-14 11:52 [PATCH] omapdrm: Make fixed resolution panels work Archit Taneja
@ 2013-02-14 15:54 ` Rob Clark
2013-02-18 7:23 ` Archit Taneja
0 siblings, 1 reply; 5+ messages in thread
From: Rob Clark @ 2013-02-14 15:54 UTC (permalink / raw)
To: Archit Taneja; +Cc: tomi.valkeinen, linux-omap, dri-devel, greg
On Thu, Feb 14, 2013 at 6:52 AM, Archit Taneja <archit@ti.com> 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.
>
> I tested this with picodlp on a OMAP4 sdp board. I couldn't get any other
> working boards with fixed DPI panels. I could try the DSI video mode panel
> on an OMAP5 sevm, but that will require me to patch a lot of things to get
> omap5 dss work with DT. I can try if needed.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/staging/omapdrm/omap_connector.c | 10 ++++++++--
> drivers/staging/omapdrm/omap_encoder.c | 6 ++++--
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/omapdrm/omap_connector.c b/drivers/staging/omapdrm/omap_connector.c
> index 4cc9ee7..839c6e4 100644
> --- a/drivers/staging/omapdrm/omap_connector.c
> +++ b/drivers/staging/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;
hmm, why not just have a default detect fxn for panel drivers which
always returns true? Seems a bit cleaner.. otherwise, I guess we
could just change omap_connector so that if dssdev->detect is null,
assume always connected. Seems maybe a slightly better way since
currently omap_connector doesn't really know too much about different
sorts of panels. But I'm not too picky if that is a big pain because
we probably end up changing all this as CFP starts coming into
existence.
Same goes for check_timings/set_timings.. it seems a bit cleaner just
to have default fxns in the panel drivers that do-the-right-thing,
although I suppose it might be a bit more convenient for out-of-tree
panel drivers to just assume fixed panel if these fxns are null.
BR,
-R
> } else {
> ret = connector_status_unknown;
> }
> @@ -189,12 +194,13 @@ 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)) {
> + r = dssdrv->check_timings ? dssdrv->check_timings(dssdev, &timings) : 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/staging/omapdrm/omap_encoder.c b/drivers/staging/omapdrm/omap_encoder.c
> index e053160..871af12e 100644
> --- a/drivers/staging/omapdrm/omap_encoder.c
> +++ b/drivers/staging/omapdrm/omap_encoder.c
> @@ -128,13 +128,15 @@ int omap_encoder_update(struct drm_encoder *encoder,
>
> dssdev->output->manager = mgr;
>
> - ret = dssdrv->check_timings(dssdev, timings);
> + ret = dssdrv->check_timings ?
> + dssdrv->check_timings(dssdev, timings) : 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.9.5
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] omapdrm: Make fixed resolution panels work
2013-02-14 15:54 ` Rob Clark
@ 2013-02-18 7:23 ` Archit Taneja
2013-02-18 8:31 ` Tomi Valkeinen
0 siblings, 1 reply; 5+ messages in thread
From: Archit Taneja @ 2013-02-18 7:23 UTC (permalink / raw)
To: Rob Clark, tomi.valkeinen; +Cc: linux-omap, dri-devel, greg
On Thursday 14 February 2013 09:24 PM, Rob Clark wrote:
> On Thu, Feb 14, 2013 at 6:52 AM, Archit Taneja <archit@ti.com> 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.
>>
>> I tested this with picodlp on a OMAP4 sdp board. I couldn't get any other
>> working boards with fixed DPI panels. I could try the DSI video mode panel
>> on an OMAP5 sevm, but that will require me to patch a lot of things to get
>> omap5 dss work with DT. I can try if needed.
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>
>> ---
>> drivers/staging/omapdrm/omap_connector.c | 10 ++++++++--
>> drivers/staging/omapdrm/omap_encoder.c | 6 ++++--
>> 2 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/omapdrm/omap_connector.c b/drivers/staging/omapdrm/omap_connector.c
>> index 4cc9ee7..839c6e4 100644
>> --- a/drivers/staging/omapdrm/omap_connector.c
>> +++ b/drivers/staging/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;
>
> hmm, why not just have a default detect fxn for panel drivers which
> always returns true? Seems a bit cleaner.. otherwise, I guess we
> could just change omap_connector so that if dssdev->detect is null,
> assume always connected. Seems maybe a slightly better way since
> currently omap_connector doesn't really know too much about different
> sorts of panels. But I'm not too picky if that is a big pain because
> we probably end up changing all this as CFP starts coming into
> existence.
>
> Same goes for check_timings/set_timings.. it seems a bit cleaner just
> to have default fxns in the panel drivers that do-the-right-thing,
> although I suppose it might be a bit more convenient for out-of-tree
> panel drivers to just assume fixed panel if these fxns are null.
Maybe having default functions in omapdss might be a better idea. There
is an option of adding default functions in omap_dss_register_driver()
(drivers/video/omap2/dss/core.c).
Tomi, do you think it's fine to add some more default panel driver funcs?
Archit
>
> BR,
> -R
>
>> } else {
>> ret = connector_status_unknown;
>> }
>> @@ -189,12 +194,13 @@ 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)) {
>> + r = dssdrv->check_timings ? dssdrv->check_timings(dssdev, &timings) : 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/staging/omapdrm/omap_encoder.c b/drivers/staging/omapdrm/omap_encoder.c
>> index e053160..871af12e 100644
>> --- a/drivers/staging/omapdrm/omap_encoder.c
>> +++ b/drivers/staging/omapdrm/omap_encoder.c
>> @@ -128,13 +128,15 @@ int omap_encoder_update(struct drm_encoder *encoder,
>>
>> dssdev->output->manager = mgr;
>>
>> - ret = dssdrv->check_timings(dssdev, timings);
>> + ret = dssdrv->check_timings ?
>> + dssdrv->check_timings(dssdev, timings) : 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.9.5
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] omapdrm: Make fixed resolution panels work
2013-02-18 7:23 ` Archit Taneja
@ 2013-02-18 8:31 ` Tomi Valkeinen
2013-02-18 11:30 ` Archit Taneja
0 siblings, 1 reply; 5+ messages in thread
From: Tomi Valkeinen @ 2013-02-18 8:31 UTC (permalink / raw)
To: Archit Taneja; +Cc: Rob Clark, linux-omap, dri-devel, greg
[-- Attachment #1: Type: text/plain, Size: 3158 bytes --]
On 2013-02-18 09:23, Archit Taneja wrote:
>>> diff --git a/drivers/staging/omapdrm/omap_connector.c
>>> b/drivers/staging/omapdrm/omap_connector.c
>>> index 4cc9ee7..839c6e4 100644
>>> --- a/drivers/staging/omapdrm/omap_connector.c
>>> +++ b/drivers/staging/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;
>>
>> hmm, why not just have a default detect fxn for panel drivers which
>> always returns true? Seems a bit cleaner.. otherwise, I guess we
>> could just change omap_connector so that if dssdev->detect is null,
>> assume always connected. Seems maybe a slightly better way since
>> currently omap_connector doesn't really know too much about different
>> sorts of panels. But I'm not too picky if that is a big pain because
>> we probably end up changing all this as CFP starts coming into
>> existence.
>>
>> Same goes for check_timings/set_timings.. it seems a bit cleaner just
>> to have default fxns in the panel drivers that do-the-right-thing,
>> although I suppose it might be a bit more convenient for out-of-tree
>> panel drivers to just assume fixed panel if these fxns are null.
Yeah, I can't make my mind about null pointer. It's nice to inform with
a null pointer that the panel doesn't support the given operation, or
that it doesn't make sense, but handling the null value makes the code a
bit complex.
For example, our VENC driver doesn't know if there's a TV connected or
not, so neither true or false as connect return value makes sense there.
Or, for a DSI command mode panel, set_timings doesn't really make much
sense.
> Maybe having default functions in omapdss might be a better idea. There
> is an option of adding default functions in omap_dss_register_driver()
> (drivers/video/omap2/dss/core.c).
>
> Tomi, do you think it's fine to add some more default panel driver funcs?
We can add default funcs. However, adding them in
omap_dss_register_driver is not so nice, as it makes the omap_dss_driver
(which is essentially an "ops" struct) non-constable. Instead, we should
move the setting of the default funcs to the drivers.
If I'm not mistaken, we could manage that with a helper macro. Assigning
a value to the same field of a struct should be ok by the compiler, and
should cause only the last assignment to be taken into use. So:
static struct foo zab = {
.bar = 123, /* ignored */
.bar = 234, /* used */
};
And so we could have:
static struct omap_dss_driver my_panel_driver = {
OMAPDSS_DEFAULT_PANEL_OPS, /* macro for default ops */
.enable = my_panel_enable,
/* ... */
};
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] omapdrm: Make fixed resolution panels work
2013-02-18 8:31 ` Tomi Valkeinen
@ 2013-02-18 11:30 ` Archit Taneja
0 siblings, 0 replies; 5+ messages in thread
From: Archit Taneja @ 2013-02-18 11:30 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Rob Clark, linux-omap, dri-devel, greg
On Monday 18 February 2013 02:01 PM, Tomi Valkeinen wrote:
> On 2013-02-18 09:23, Archit Taneja wrote:
>
>>>> diff --git a/drivers/staging/omapdrm/omap_connector.c
>>>> b/drivers/staging/omapdrm/omap_connector.c
>>>> index 4cc9ee7..839c6e4 100644
>>>> --- a/drivers/staging/omapdrm/omap_connector.c
>>>> +++ b/drivers/staging/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;
>>>
>>> hmm, why not just have a default detect fxn for panel drivers which
>>> always returns true? Seems a bit cleaner.. otherwise, I guess we
>>> could just change omap_connector so that if dssdev->detect is null,
>>> assume always connected. Seems maybe a slightly better way since
>>> currently omap_connector doesn't really know too much about different
>>> sorts of panels. But I'm not too picky if that is a big pain because
>>> we probably end up changing all this as CFP starts coming into
>>> existence.
>>>
>>> Same goes for check_timings/set_timings.. it seems a bit cleaner just
>>> to have default fxns in the panel drivers that do-the-right-thing,
>>> although I suppose it might be a bit more convenient for out-of-tree
>>> panel drivers to just assume fixed panel if these fxns are null.
>
> Yeah, I can't make my mind about null pointer. It's nice to inform with
> a null pointer that the panel doesn't support the given operation, or
> that it doesn't make sense, but handling the null value makes the code a
> bit complex.
>
> For example, our VENC driver doesn't know if there's a TV connected or
> not, so neither true or false as connect return value makes sense there.
> Or, for a DSI command mode panel, set_timings doesn't really make much
> sense.
>
>> Maybe having default functions in omapdss might be a better idea. There
>> is an option of adding default functions in omap_dss_register_driver()
>> (drivers/video/omap2/dss/core.c).
>>
>> Tomi, do you think it's fine to add some more default panel driver funcs?
>
> We can add default funcs. However, adding them in
> omap_dss_register_driver is not so nice, as it makes the omap_dss_driver
> (which is essentially an "ops" struct) non-constable. Instead, we should
> move the setting of the default funcs to the drivers.
>
> If I'm not mistaken, we could manage that with a helper macro. Assigning
> a value to the same field of a struct should be ok by the compiler, and
> should cause only the last assignment to be taken into use. So:
>
> static struct foo zab = {
> .bar = 123, /* ignored */
> .bar = 234, /* used */
> };
>
> And so we could have:
>
> static struct omap_dss_driver my_panel_driver = {
> OMAPDSS_DEFAULT_PANEL_OPS, /* macro for default ops */
>
> .enable = my_panel_enable,
> /* ... */
> };
This sounds nice, but if the panel driver ops are going to be changed
again in CPF, then it might make sense to do this later, depending on
how it CPF handles default ops in panel drivers.
Maybe we should leave this patch as-is, since we know it will be
modified later.
Archit
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-02-18 11:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-14 11:52 [PATCH] omapdrm: Make fixed resolution panels work Archit Taneja
2013-02-14 15:54 ` Rob Clark
2013-02-18 7:23 ` Archit Taneja
2013-02-18 8:31 ` Tomi Valkeinen
2013-02-18 11:30 ` Archit Taneja
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.