| Message ID | 1363093583-28285-1-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2013-03-12 15:06, 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 <archit@ti.com> > --- > Note: In v2, we make sure that the mode passed on to omapdrm matches the timings > of the fixed resolution panel. > > 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..a72fedd 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; I have to say I don't like this. We shouldn't care about the type here. I think it's better just to default to connected if there's no detect function (or unknown? I'm not sure what is the practical difference). If it works fine without using dssdev->type, we have one less place to worry when doing dss dev model changes =). > } 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; > + > + dssdrv->get_timings(dssdev, &t); > + > + if (memcmp(&timings, &t, sizeof(struct omap_video_timings))) > + r = -EINVAL; > + else > + r = 0; memcmp on two structs is often not a good idea. There could be padding bytes there, with uninitialized data. I'm not sure if that's the case here, though, but it could well change any time (perhaps even depending on compiler version). I'm still pondering whether it'd just be simpler to require all the dssdrv ops to be filled, probably using a helper macro in the panel drivers... Did you consider that approach? I'm not saying to go for it, I'm saying I can't make my mind which would be better =). Tomi
On Tuesday 12 March 2013 07:36 PM, Tomi Valkeinen wrote: > On 2013-03-12 15:06, 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 <archit@ti.com> >> --- >> Note: In v2, we make sure that the mode passed on to omapdrm matches the timings >> of the fixed resolution panel. >> >> 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..a72fedd 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; > > I have to say I don't like this. We shouldn't care about the type here. > I think it's better just to default to connected if there's no detect > function (or unknown? I'm not sure what is the practical difference). > > If it works fine without using dssdev->type, we have one less place to > worry when doing dss dev model changes =). > >> } 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; >> + >> + dssdrv->get_timings(dssdev, &t); >> + >> + if (memcmp(&timings, &t, sizeof(struct omap_video_timings))) >> + r = -EINVAL; >> + else >> + r = 0; > > memcmp on two structs is often not a good idea. There could be padding > bytes there, with uninitialized data. I'm not sure if that's the case > here, though, but it could well change any time (perhaps even depending > on compiler version). I saw usage of memcmp on structs in the kernel, but then most of them were in drivers and not core, and could be mistakes :) adding '__attribute__((packed))' to omap_video_timings might be helpful I suppose. But I don't know if it's safe to do a memcmp even with a packed struct. > > I'm still pondering whether it'd just be simpler to require all the > dssdrv ops to be filled, probably using a helper macro in the panel > drivers... Did you consider that approach? I'm not saying to go for it, > I'm saying I can't make my mind which would be better =). I didn't consider it mainly because I thought we were going to get rid of our private omapdss panel drivers with CDF panels, and efforts in fixing it wouldn't be much useful. But if there isn't any other good alternative, then I can try to see what macros look like. Of course, simpler options for this patch would be to do a manual compare of the fields instead of a memcmp, or adding default ops in omap_dss_register_driver. One thing about common panel driver functions in general is that they won't use the panel driver's locking. So if a panel driver doesn't create a get_timings() op assuming that omapdss will make a default func for it, we would miss out on the panel lock. I don't know if that's really bad, and doing a memcmp or anything else in omapdrm also doesn't help with this case. Archit
On 2013-03-12 16:38, Archit Taneja wrote: >> memcmp on two structs is often not a good idea. There could be padding >> bytes there, with uninitialized data. I'm not sure if that's the case >> here, though, but it could well change any time (perhaps even depending >> on compiler version). > > I saw usage of memcmp on structs in the kernel, but then most of them > were in drivers and not core, and could be mistakes :) > > adding '__attribute__((packed))' to omap_video_timings might be helpful > I suppose. But I don't know if it's safe to do a memcmp even with a > packed struct. I think it's safe to use memcmp if you know that both structs have been initialized to zero with memset. >> I'm still pondering whether it'd just be simpler to require all the >> dssdrv ops to be filled, probably using a helper macro in the panel >> drivers... Did you consider that approach? I'm not saying to go for it, >> I'm saying I can't make my mind which would be better =). > > I didn't consider it mainly because I thought we were going to get rid > of our private omapdss panel drivers with CDF panels, and efforts in > fixing it wouldn't be much useful. But if there isn't any other good > alternative, then I can try to see what macros look like. Well, even if I was slightly optimistic earlier, I now have a feeling CDF may take a while. I think we should just go for omapdss dev model change first. One thing to note about the ops is that NULL is currently used to convey information, something like "this ops is not possible or valid". So adding all the ops may not quite work. For example, adding update op for auto-update panels could tell that the panel supports manual update. (Well, for that particular case we have a flag, but you get the idea). But if we can add only some of the ops to the drivers, and there is no information lost when we won't have NULLs, I guess that could be the simplest option. Then we don't need to add extra code in each place we use the ops. > Of course, simpler options for this patch would be to do a manual > compare of the fields instead of a memcmp, or adding default ops in > omap_dss_register_driver. Adding default ops in omap_dss_register_driver() is not good. It prevents us from having the ops as const in the future, and is also not possible when we either move to CDF or change the omapdss dev model. So I think either we need to handle the NULLs as you do in this patch, or add the ops to the panels. But the ops could still be the default versions offered by the omapdss. > One thing about common panel driver functions in general is that they > won't use the panel driver's locking. So if a panel driver doesn't > create a get_timings() op assuming that omapdss will make a default func > for it, we would miss out on the panel lock. I don't know if that's > really bad, and doing a memcmp or anything else in omapdrm also doesn't > help with this case. That's true. The locking is a bit of a mess (read: broken =) anyway currently. Tomi
On Tuesday 12 March 2013 08:23 PM, Tomi Valkeinen wrote: > On 2013-03-12 16:38, Archit Taneja wrote: > >>> memcmp on two structs is often not a good idea. There could be padding >>> bytes there, with uninitialized data. I'm not sure if that's the case >>> here, though, but it could well change any time (perhaps even depending >>> on compiler version). >> >> I saw usage of memcmp on structs in the kernel, but then most of them >> were in drivers and not core, and could be mistakes :) >> >> adding '__attribute__((packed))' to omap_video_timings might be helpful >> I suppose. But I don't know if it's safe to do a memcmp even with a >> packed struct. > > I think it's safe to use memcmp if you know that both structs have been > initialized to zero with memset. > >>> I'm still pondering whether it'd just be simpler to require all the >>> dssdrv ops to be filled, probably using a helper macro in the panel >>> drivers... Did you consider that approach? I'm not saying to go for it, >>> I'm saying I can't make my mind which would be better =). >> >> I didn't consider it mainly because I thought we were going to get rid >> of our private omapdss panel drivers with CDF panels, and efforts in >> fixing it wouldn't be much useful. But if there isn't any other good >> alternative, then I can try to see what macros look like. > > Well, even if I was slightly optimistic earlier, I now have a feeling > CDF may take a while. I think we should just go for omapdss dev model > change first. > > One thing to note about the ops is that NULL is currently used to convey > information, something like "this ops is not possible or valid". So > adding all the ops may not quite work. For example, adding update op for > auto-update panels could tell that the panel supports manual update. > (Well, for that particular case we have a flag, but you get the idea). > > But if we can add only some of the ops to the drivers, and there is no > information lost when we won't have NULLs, I guess that could be the > simplest option. Then we don't need to add extra code in each place we > use the ops. > >> Of course, simpler options for this patch would be to do a manual >> compare of the fields instead of a memcmp, or adding default ops in >> omap_dss_register_driver. > > Adding default ops in omap_dss_register_driver() is not good. It > prevents us from having the ops as const in the future, and is also not > possible when we either move to CDF or change the omapdss dev model. > > So I think either we need to handle the NULLs as you do in this patch, > or add the ops to the panels. But the ops could still be the default > versions offered by the omapdss. I was trying to come up with a macro which could add default ops to the omap_dss_driver. It isn't straight forward as I thought, because you need to choose either the default op, or the panel driver's op if it exists. For example, I can't create a macro which adds an op for get_timings() to all panel drivers, the panel drivers which already this op defined will have 2 instances of get_timings() in the omap_dss_driver struct. I have been looking around in the kernel to see how undeclared ops in a struct are generally dealt with. Till now, I've noticed that the code which uses this ops takes the responsibility to check whether they are NULL or not. The easiest way would be to have these default funcs inlined in a header, and every panel driver's omap_dss_driver struct fills in the default op. This way we can make the driver ops const. Do you have any idea of a macro which could do the same as above, and leads to less addition of code? Archit > >> One thing about common panel driver functions in general is that they >> won't use the panel driver's locking. So if a panel driver doesn't >> create a get_timings() op assuming that omapdss will make a default func >> for it, we would miss out on the panel lock. I don't know if that's >> really bad, and doing a memcmp or anything else in omapdrm also doesn't >> help with this case. > > That's true. The locking is a bit of a mess (read: broken =) anyway > currently. > > Tomi > >
On 2013-03-19 08:45, Archit Taneja wrote: > I was trying to come up with a macro which could add default ops to the > omap_dss_driver. It isn't straight forward as I thought, because you > need to choose either the default op, or the panel driver's op if it > exists. For example, I can't create a macro which adds an op for > get_timings() to all panel drivers, the panel drivers which already this > op defined will have 2 instances of get_timings() in the omap_dss_driver > struct. Yep, I noticed the same a few days ago. > I have been looking around in the kernel to see how undeclared ops in a > struct are generally dealt with. Till now, I've noticed that the code > which uses this ops takes the responsibility to check whether they are > NULL or not. > > The easiest way would be to have these default funcs inlined in a > header, and every panel driver's omap_dss_driver struct fills in the > default op. This way we can make the driver ops const. Do you have any > idea of a macro which could do the same as above, and leads to less > addition of code? Why would they need to be inlined? Another option would be to create global funcs that are used to call the ops. So instead of: dssdev->dssdrv->foo(dssdev) the user would call this function: int dss_foo(struct omap_dss_device *dssdev) { if (dssdev->dssdrv->foo == NULL) return 0; /* or error, depending on case */ return dssdev->dssdrv->foo(dssdev); } But that'd require adding a bunch of functions, and changing all the callers. I think the safest way to fix this for now is to add the checks into omapdrm as you do in your original patch. If we go for some other route, I fear that omapfb/omap_vout could be affected, as it could presume that an op being NULL or non-NULL means something. If we change the ops to be always non-NULL, we should go over the uses of those ops to verify they work correctly. Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c index c451c41..a72fedd 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; + + 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..47971c2 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; + + 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; }
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 <archit@ti.com> --- Note: In v2, we make sure that the mode passed on to omapdrm matches the timings of the fixed resolution panel. drivers/gpu/drm/omapdrm/omap_connector.c | 27 +++++++++++++++++++++++++-- drivers/gpu/drm/omapdrm/omap_encoder.c | 17 +++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-)