| Message ID | 20191114093950.4101-1-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/omap: fix am4 evm lcd | expand |
Hi Tony, Thierry, Laurent, Any thoughts on the below points? I think yet another option is to write some omap boot time quirks code, which looks at the DT data, and changes the panel compatible string (for 1), and removes the timings node (for 2). Tomi On 14/11/2019 11:39, Tomi Valkeinen wrote: > Hi Tony, Thierry, Laurent, > > After the recent change of moving from omapdrm specific panel-dpi driver > to the DRM simple panel, AM4 EVM/ePOS's panel is not working quite > right. This series has fixes for it, but I'm not sure if these are the > right ways to fix the issues, so comments welcome. > > 1) Panel driver is not probed. With omapdrm's panel-dpi, the match > happened with "panel-dpi" compatible string. Now with panel-simple, the > match should happen with the panel model compatible string, which is > "osddisplays,osd057T0559-34ts" in the DT file. However, no such > compatible exists in panel-simple. > > Interestingly, the actual panel at least on my EVMs and ePOSes is not > osd057T0559-34ts, but osd070t1718-19ts. Also, I was unable to find any > information about osd057T0559-34ts. I don't know the history with this, > so it is possible that the early versions of the boards did have > osd057T0559-34ts, but was later changed to osd070t1718-19ts. > > As osd070t1718-19ts is supported by panel-simple, changing the > compatible string to osd070t1718-19ts in the DT file solves this one. > > 2) Timings in DT file cause a kernel warning. Omapdrm's panel-dpi used > video timings from the DT file, so they are present in all the DT files. > panel-simple uses timings from a table in the panel-simple driver, but > gives a kernel warning if the DT file contains timings. > > This can be solved by removing the timings from the DT file. > > 3) Sync drive edge is not right. This one might have been present also > with panel-dpi, I didn't verify. The problem is that the panel-simple > data for osddisplays_osd070t1718_19ts defines bus_flags for DE polarity > and pixdata edge, but not for sync edge. > > The datasheet for the panel does not give any hint on what the edge > should be. Omapdrm defaults to driving syncs on falling edge, which > caused the image to be shifted one pixel to the right. > > Adding DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE bus_flag solves the problem. AM5 > EVM also has the same panel with the same behavior. > > ==== > > The reason I'm not sure if the 1) and 2) fixes are correct is that > they're breaking DT compatibility. Should we instead make changes to > panel-simple to keep the same DT files working? > > This would mean adding a new entry for the osd057T0559-34ts panel, but > as we don't have datasheet for it, I think we could just append the > compatible string to osd070t1718-19t's data. > > It would also mean doing some change to the panel-simple code that gives > the warning about timings in DT data. This might make sense, as I think > we have other DT files with video timings too. > > For 3), I think the patch is fine, but I'm not sure if the display > controller driver should be able to deduce the sync drive edge from the > pixdata drive edge. Are they usually the same? I have no idea... > > Tomi > > Tomi Valkeinen (3): > ARM: dts: am437x-gp/epos-evm: fix panel compatible > ARM: dts: am437x-gp/epos-evm: drop unused panel timings > drm/panel: simple: fix osd070t1718_19ts sync drive edge > > arch/arm/boot/dts/am437x-gp-evm.dts | 18 +----------------- > arch/arm/boot/dts/am43x-epos-evm.dts | 18 +----------------- > drivers/gpu/drm/panel/panel-simple.c | 3 ++- > 3 files changed, 4 insertions(+), 35 deletions(-) > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >
Hi * Tomi Valkeinen <tomi.valkeinen@ti.com> [191127 12:59]: > Hi Tony, Thierry, Laurent, > > Any thoughts on the below points? > I think yet another option is to write some omap boot time quirks code, > which looks at the DT data, and changes the panel compatible string (for 1), > and removes the timings node (for 2). Nah, seems we can just update the compatible. > On 14/11/2019 11:39, Tomi Valkeinen wrote: > > 1) Panel driver is not probed. With omapdrm's panel-dpi, the match > > happened with "panel-dpi" compatible string. Now with panel-simple, the > > match should happen with the panel model compatible string, which is > > "osddisplays,osd057T0559-34ts" in the DT file. However, no such > > compatible exists in panel-simple. > > > > Interestingly, the actual panel at least on my EVMs and ePOSes is not > > osd057T0559-34ts, but osd070t1718-19ts. Also, I was unable to find any > > information about osd057T0559-34ts. I don't know the history with this, > > so it is possible that the early versions of the boards did have > > osd057T0559-34ts, but was later changed to osd070t1718-19ts. I guess you could keep the old compatible there too if really needed. But then again if the old compatible is known to be incorrect, it should be just updated. So it looks good to me for the dts changes. Do you want me to pick them into fixes as it seems that the panel driver fix is a separate issue? Regards, Tony
Hi Tony, On 27/11/2019 17:45, Tony Lindgren wrote: >>> Interestingly, the actual panel at least on my EVMs and ePOSes is not >>> osd057T0559-34ts, but osd070t1718-19ts. Also, I was unable to find any >>> information about osd057T0559-34ts. I don't know the history with this, >>> so it is possible that the early versions of the boards did have >>> osd057T0559-34ts, but was later changed to osd070t1718-19ts. > > I guess you could keep the old compatible there too if really needed. > But then again if the old compatible is known to be incorrect, it > should be just updated. > > So it looks good to me for the dts changes. Do you want me to pick > them into fixes as it seems that the panel driver fix is a separate > issue? Yes, the third patch can be handled separately, so please pick the first two ones. Tomi
Hi Tomi, On Thu, Nov 14, 2019 at 11:39:47AM +0200, Tomi Valkeinen wrote: > Hi Tony, Thierry, Laurent, > > After the recent change of moving from omapdrm specific panel-dpi driver > to the DRM simple panel, AM4 EVM/ePOS's panel is not working quite > right. This series has fixes for it, but I'm not sure if these are the > right ways to fix the issues, so comments welcome. > > 1) Panel driver is not probed. With omapdrm's panel-dpi, the match > happened with "panel-dpi" compatible string. Now with panel-simple, the > match should happen with the panel model compatible string, which is > "osddisplays,osd057T0559-34ts" in the DT file. However, no such > compatible exists in panel-simple. I've also noticed that we have a few other omap-based platforms that got broken, for the same reason :-( We're missing driver support for innolux,at070tn83, samsung,lte430wq-f0c and startek,startek-kd050c, and we have a few nodes that use panel-dpi without any more precise compatible string. > Interestingly, the actual panel at least on my EVMs and ePOSes is not > osd057T0559-34ts, but osd070t1718-19ts. Also, I was unable to find any > information about osd057T0559-34ts. I don't know the history with this, > so it is possible that the early versions of the boards did have > osd057T0559-34ts, but was later changed to osd070t1718-19ts. > > As osd070t1718-19ts is supported by panel-simple, changing the > compatible string to osd070t1718-19ts in the DT file solves this one. > > 2) Timings in DT file cause a kernel warning. Omapdrm's panel-dpi used > video timings from the DT file, so they are present in all the DT files. > panel-simple uses timings from a table in the panel-simple driver, but > gives a kernel warning if the DT file contains timings. > > This can be solved by removing the timings from the DT file. > > 3) Sync drive edge is not right. This one might have been present also > with panel-dpi, I didn't verify. The problem is that the panel-simple > data for osddisplays_osd070t1718_19ts defines bus_flags for DE polarity > and pixdata edge, but not for sync edge. > > The datasheet for the panel does not give any hint on what the edge > should be. Omapdrm defaults to driving syncs on falling edge, which > caused the image to be shifted one pixel to the right. > > Adding DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE bus_flag solves the problem. AM5 > EVM also has the same panel with the same behavior. > > ==== > > The reason I'm not sure if the 1) and 2) fixes are correct is that > they're breaking DT compatibility. Should we instead make changes to > panel-simple to keep the same DT files working? That's tempting, as breaking DT is quite bad, but that would require adding a match on panel-dpi, and parsing timings in the panel-simple driver. Thierry has always been opposed to that as far as I can tell, and even if I don't share his point of view, I don't want to move in this direction without a consensus. Your series is fine in my opinion, as even if we decide to handle backward compatibility in this case, changing the DT files in mainline is still the right way to go (if only to avoid giving wrong examples). > This would mean adding a new entry for the osd057T0559-34ts panel, but > as we don't have datasheet for it, I think we could just append the > compatible string to osd070t1718-19t's data. > > It would also mean doing some change to the panel-simple code that gives > the warning about timings in DT data. This might make sense, as I think > we have other DT files with video timings too. > > For 3), I think the patch is fine, but I'm not sure if the display > controller driver should be able to deduce the sync drive edge from the > pixdata drive edge. Are they usually the same? I have no idea... > > Tomi Valkeinen (3): > ARM: dts: am437x-gp/epos-evm: fix panel compatible > ARM: dts: am437x-gp/epos-evm: drop unused panel timings > drm/panel: simple: fix osd070t1718_19ts sync drive edge > > arch/arm/boot/dts/am437x-gp-evm.dts | 18 +----------------- > arch/arm/boot/dts/am43x-epos-evm.dts | 18 +----------------- > drivers/gpu/drm/panel/panel-simple.c | 3 ++- > 3 files changed, 4 insertions(+), 35 deletions(-)
On Mon, Dec 2, 2019 at 7:13 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Tomi, > > On Thu, Nov 14, 2019 at 11:39:47AM +0200, Tomi Valkeinen wrote: > > Hi Tony, Thierry, Laurent, > > > > After the recent change of moving from omapdrm specific panel-dpi driver > > to the DRM simple panel, AM4 EVM/ePOS's panel is not working quite > > right. This series has fixes for it, but I'm not sure if these are the > > right ways to fix the issues, so comments welcome. > > > > 1) Panel driver is not probed. With omapdrm's panel-dpi, the match > > happened with "panel-dpi" compatible string. Now with panel-simple, the > > match should happen with the panel model compatible string, which is > > "osddisplays,osd057T0559-34ts" in the DT file. However, no such > > compatible exists in panel-simple. > > I've also noticed that we have a few other omap-based platforms that > got broken, for the same reason :-( We're missing driver support for > innolux,at070tn83, samsung,lte430wq-f0c and startek,startek-kd050c, and > we have a few nodes that use panel-dpi without any more precise > compatible string. > > > Interestingly, the actual panel at least on my EVMs and ePOSes is not > > osd057T0559-34ts, but osd070t1718-19ts. Also, I was unable to find any > > information about osd057T0559-34ts. I don't know the history with this, > > so it is possible that the early versions of the boards did have > > osd057T0559-34ts, but was later changed to osd070t1718-19ts. > > > > As osd070t1718-19ts is supported by panel-simple, changing the > > compatible string to osd070t1718-19ts in the DT file solves this one. > > > > 2) Timings in DT file cause a kernel warning. Omapdrm's panel-dpi used > > video timings from the DT file, so they are present in all the DT files. > > panel-simple uses timings from a table in the panel-simple driver, but > > gives a kernel warning if the DT file contains timings. > > > > This can be solved by removing the timings from the DT file. > > > > 3) Sync drive edge is not right. This one might have been present also > > with panel-dpi, I didn't verify. The problem is that the panel-simple > > data for osddisplays_osd070t1718_19ts defines bus_flags for DE polarity > > and pixdata edge, but not for sync edge. > > > > The datasheet for the panel does not give any hint on what the edge > > should be. Omapdrm defaults to driving syncs on falling edge, which > > caused the image to be shifted one pixel to the right. > > > > Adding DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE bus_flag solves the problem. AM5 > > EVM also has the same panel with the same behavior. > > > > ==== > > > > The reason I'm not sure if the 1) and 2) fixes are correct is that > > they're breaking DT compatibility. Should we instead make changes to > > panel-simple to keep the same DT files working? > > That's tempting, as breaking DT is quite bad, but that would require > adding a match on panel-dpi, and parsing timings in the panel-simple > driver. Thierry has always been opposed to that as far as I can tell, > and even if I don't share his point of view, I don't want to move in > this direction without a consensus. What about a generic driver separate from the simple panel driver that does just the panel-dpi parsing? If we exported the necessary functions from simple-panel, we could call them from the panel-dpi parser and not have to re-invent the functions to enable power, gpio or back light and/or fix them each time they get updated. adam > > Your series is fine in my opinion, as even if we decide to handle > backward compatibility in this case, changing the DT files in mainline > is still the right way to go (if only to avoid giving wrong examples). > > > This would mean adding a new entry for the osd057T0559-34ts panel, but > > as we don't have datasheet for it, I think we could just append the > > compatible string to osd070t1718-19t's data. > > > > It would also mean doing some change to the panel-simple code that gives > > the warning about timings in DT data. This might make sense, as I think > > we have other DT files with video timings too. > > > > For 3), I think the patch is fine, but I'm not sure if the display > > controller driver should be able to deduce the sync drive edge from the > > pixdata drive edge. Are they usually the same? I have no idea... > > > > Tomi Valkeinen (3): > > ARM: dts: am437x-gp/epos-evm: fix panel compatible > > ARM: dts: am437x-gp/epos-evm: drop unused panel timings > > drm/panel: simple: fix osd070t1718_19ts sync drive edge > > > > arch/arm/boot/dts/am437x-gp-evm.dts | 18 +----------------- > > arch/arm/boot/dts/am43x-epos-evm.dts | 18 +----------------- > > drivers/gpu/drm/panel/panel-simple.c | 3 ++- > > 3 files changed, 4 insertions(+), 35 deletions(-) > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Adam, On Mon, Dec 02, 2019 at 07:24:09AM -0600, Adam Ford wrote: > On Mon, Dec 2, 2019 at 7:13 AM Laurent Pinchart wrote: > > On Thu, Nov 14, 2019 at 11:39:47AM +0200, Tomi Valkeinen wrote: > > > Hi Tony, Thierry, Laurent, > > > > > > After the recent change of moving from omapdrm specific panel-dpi driver > > > to the DRM simple panel, AM4 EVM/ePOS's panel is not working quite > > > right. This series has fixes for it, but I'm not sure if these are the > > > right ways to fix the issues, so comments welcome. > > > > > > 1) Panel driver is not probed. With omapdrm's panel-dpi, the match > > > happened with "panel-dpi" compatible string. Now with panel-simple, the > > > match should happen with the panel model compatible string, which is > > > "osddisplays,osd057T0559-34ts" in the DT file. However, no such > > > compatible exists in panel-simple. > > > > I've also noticed that we have a few other omap-based platforms that > > got broken, for the same reason :-( We're missing driver support for > > innolux,at070tn83, samsung,lte430wq-f0c and startek,startek-kd050c, and > > we have a few nodes that use panel-dpi without any more precise > > compatible string. > > > > > Interestingly, the actual panel at least on my EVMs and ePOSes is not > > > osd057T0559-34ts, but osd070t1718-19ts. Also, I was unable to find any > > > information about osd057T0559-34ts. I don't know the history with this, > > > so it is possible that the early versions of the boards did have > > > osd057T0559-34ts, but was later changed to osd070t1718-19ts. > > > > > > As osd070t1718-19ts is supported by panel-simple, changing the > > > compatible string to osd070t1718-19ts in the DT file solves this one. > > > > > > 2) Timings in DT file cause a kernel warning. Omapdrm's panel-dpi used > > > video timings from the DT file, so they are present in all the DT files. > > > panel-simple uses timings from a table in the panel-simple driver, but > > > gives a kernel warning if the DT file contains timings. > > > > > > This can be solved by removing the timings from the DT file. > > > > > > 3) Sync drive edge is not right. This one might have been present also > > > with panel-dpi, I didn't verify. The problem is that the panel-simple > > > data for osddisplays_osd070t1718_19ts defines bus_flags for DE polarity > > > and pixdata edge, but not for sync edge. > > > > > > The datasheet for the panel does not give any hint on what the edge > > > should be. Omapdrm defaults to driving syncs on falling edge, which > > > caused the image to be shifted one pixel to the right. > > > > > > Adding DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE bus_flag solves the problem. AM5 > > > EVM also has the same panel with the same behavior. > > > > > > ==== > > > > > > The reason I'm not sure if the 1) and 2) fixes are correct is that > > > they're breaking DT compatibility. Should we instead make changes to > > > panel-simple to keep the same DT files working? > > > > That's tempting, as breaking DT is quite bad, but that would require > > adding a match on panel-dpi, and parsing timings in the panel-simple > > driver. Thierry has always been opposed to that as far as I can tell, > > and even if I don't share his point of view, I don't want to move in > > this direction without a consensus. > > What about a generic driver separate from the simple panel driver that > does just the panel-dpi parsing? A separate driver wouldn't make too much of a difference. Thierry's point was that drivers should hardcode the timings, as the compatible string gives enough information and encoding timings in DT would duplicate that information. I have a different point of view, and there are pros and cons for both options, so we haven't been able to reach an agreement. That's why I haven't pushed too hard for timings parsing from DT, regardless of whether it would be in panel-simple or in a separate driver. > If we exported the necessary functions from simple-panel, we could > call them from the panel-dpi parser and not have to re-invent the > functions to enable power, gpio or back light and/or fix them each > time they get updated. > > > Your series is fine in my opinion, as even if we decide to handle > > backward compatibility in this case, changing the DT files in mainline > > is still the right way to go (if only to avoid giving wrong examples). > > > > > This would mean adding a new entry for the osd057T0559-34ts panel, but > > > as we don't have datasheet for it, I think we could just append the > > > compatible string to osd070t1718-19t's data. > > > > > > It would also mean doing some change to the panel-simple code that gives > > > the warning about timings in DT data. This might make sense, as I think > > > we have other DT files with video timings too. > > > > > > For 3), I think the patch is fine, but I'm not sure if the display > > > controller driver should be able to deduce the sync drive edge from the > > > pixdata drive edge. Are they usually the same? I have no idea... > > > > > > Tomi Valkeinen (3): > > > ARM: dts: am437x-gp/epos-evm: fix panel compatible > > > ARM: dts: am437x-gp/epos-evm: drop unused panel timings > > > drm/panel: simple: fix osd070t1718_19ts sync drive edge > > > > > > arch/arm/boot/dts/am437x-gp-evm.dts | 18 +----------------- > > > arch/arm/boot/dts/am43x-epos-evm.dts | 18 +----------------- > > > drivers/gpu/drm/panel/panel-simple.c | 3 ++- > > > 3 files changed, 4 insertions(+), 35 deletions(-)