| Message ID | 1367435169-15025-1-git-send-email-tomasz.figa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 01, 2013 at 09:06:09PM +0200, Tomasz Figa wrote: > This patch modifies the driver to perform two stage parsing of video > timings from device tree, to get timing information as struct videomode, > which contains more data than struct fb_videomode. > > Thanks to this change, information about polarity of control signals > (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard > video timings. > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> Since the drm mode struct also contains flags for sync polarity ... why is there no direct of -> drm_mode function? Going through an fb videomode in a kms drm driver looks _really_ backwards to me. Cc'in Dave for the fun of it ;-) Cheers, Daniel > --- > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > index a1669d4..9023efa 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > @@ -21,7 +21,9 @@ > #include <linux/pm_runtime.h> > > #include <video/of_display_timing.h> > +#include <video/of_videomode.h> > #include <video/samsung_fimd.h> > +#include <video/videomode.h> > #include <drm/exynos_drm.h> > > #include "exynos_drm_drv.h" > @@ -928,18 +930,30 @@ static int fimd_probe(struct platform_device *pdev) > DRM_DEBUG_KMS("%s\n", __FILE__); > > if (pdev->dev.of_node) { > + struct videomode vm; > + > pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > if (!pdata) { > DRM_ERROR("memory allocation for pdata failed\n"); > return -ENOMEM; > } > > - ret = of_get_fb_videomode(dev->of_node, &pdata->panel.timing, > - OF_USE_NATIVE_MODE); > + ret = of_get_videomode(dev->of_node, &vm, OF_USE_NATIVE_MODE); > if (ret) { > DRM_ERROR("failed: of_get_fb_videomode() : %d\n", ret); > return ret; > } > + > + fb_videomode_from_videomode(&vm, &pdata->panel.timing); > + > + if (vm.flags & DISPLAY_FLAGS_VSYNC_LOW) > + pdata->vidcon1 |= VIDCON1_INV_VSYNC; > + if (vm.flags & DISPLAY_FLAGS_HSYNC_LOW) > + pdata->vidcon1 |= VIDCON1_INV_HSYNC; > + if (vm.flags & DISPLAY_FLAGS_DE_LOW) > + pdata->vidcon1 |= VIDCON1_INV_VDEN; > + if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) > + pdata->vidcon1 |= VIDCON1_INV_VCLK; > } else { > pdata = pdev->dev.platform_data; > if (!pdata) { > -- > 1.8.2.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On Wednesday 01 of May 2013 22:00:25 Daniel Vetter wrote: > On Wed, May 01, 2013 at 09:06:09PM +0200, Tomasz Figa wrote: > > This patch modifies the driver to perform two stage parsing of video > > timings from device tree, to get timing information as struct > > videomode, which contains more data than struct fb_videomode. > > > > Thanks to this change, information about polarity of control signals > > (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard > > video timings. > > > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> > > Since the drm mode struct also contains flags for sync polarity ... why > is there no direct of -> drm_mode function? Going through an fb > videomode in a kms drm driver looks _really_ backwards to me. > > Cc'in Dave for the fun of it ;-) Struct fb_videomode is what exynos_drm_fimd driver uses internally. Sure it should use drm_mode, but this is not really related to this patch, because the code added in this patch only fills in the pdata struct, which for compatibility reasons (the same structure is used for both fbdev and drm drivers) contains struct fb_videomode. OK, now after having a bit of fun, could we merge this patch to at least have usable support of parallel displays using this driver? Best regards, Tomasz > Cheers, Daniel > > > --- > > > > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index a1669d4..9023efa > > 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > > @@ -21,7 +21,9 @@ > > > > #include <linux/pm_runtime.h> > > > > #include <video/of_display_timing.h> > > > > +#include <video/of_videomode.h> > > > > #include <video/samsung_fimd.h> > > > > +#include <video/videomode.h> > > > > #include <drm/exynos_drm.h> > > > > #include "exynos_drm_drv.h" > > > > @@ -928,18 +930,30 @@ static int fimd_probe(struct platform_device > > *pdev)> > > DRM_DEBUG_KMS("%s\n", __FILE__); > > > > if (pdev->dev.of_node) { > > > > + struct videomode vm; > > + > > > > pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > > if (!pdata) { > > > > DRM_ERROR("memory allocation for pdata failed\n"); > > return -ENOMEM; > > > > } > > > > - ret = of_get_fb_videomode(dev->of_node, &pdata- >panel.timing, > > - OF_USE_NATIVE_MODE); > > + ret = of_get_videomode(dev->of_node, &vm, OF_USE_NATIVE_MODE); > > > > if (ret) { > > > > DRM_ERROR("failed: of_get_fb_videomode() : %d\n", ret); > > return ret; > > > > } > > > > + > > + fb_videomode_from_videomode(&vm, &pdata->panel.timing); > > + > > + if (vm.flags & DISPLAY_FLAGS_VSYNC_LOW) > > + pdata->vidcon1 |= VIDCON1_INV_VSYNC; > > + if (vm.flags & DISPLAY_FLAGS_HSYNC_LOW) > > + pdata->vidcon1 |= VIDCON1_INV_HSYNC; > > + if (vm.flags & DISPLAY_FLAGS_DE_LOW) > > + pdata->vidcon1 |= VIDCON1_INV_VDEN; > > + if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) > > + pdata->vidcon1 |= VIDCON1_INV_VCLK; > > > > } else { > > > > pdata = pdev->dev.platform_data; > > if (!pdata) {
On Wednesday 01 of May 2013 21:06:09 Tomasz Figa wrote: > This patch modifies the driver to perform two stage parsing of video > timings from device tree, to get timing information as struct videomode, > which contains more data than struct fb_videomode. > > Thanks to this change, information about polarity of control signals > (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard > video timings. > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> > --- > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index a1669d4..9023efa > 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > @@ -21,7 +21,9 @@ > #include <linux/pm_runtime.h> > > #include <video/of_display_timing.h> > +#include <video/of_videomode.h> > #include <video/samsung_fimd.h> > +#include <video/videomode.h> > #include <drm/exynos_drm.h> > > #include "exynos_drm_drv.h" > @@ -928,18 +930,30 @@ static int fimd_probe(struct platform_device > *pdev) DRM_DEBUG_KMS("%s\n", __FILE__); > > if (pdev->dev.of_node) { > + struct videomode vm; > + > pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > if (!pdata) { > DRM_ERROR("memory allocation for pdata failed\n"); > return -ENOMEM; > } > > - ret = of_get_fb_videomode(dev->of_node, &pdata- >panel.timing, > - OF_USE_NATIVE_MODE); > + ret = of_get_videomode(dev->of_node, &vm, OF_USE_NATIVE_MODE); > if (ret) { > DRM_ERROR("failed: of_get_fb_videomode() : %d\n", ret); > return ret; > } > + > + fb_videomode_from_videomode(&vm, &pdata->panel.timing); > + > + if (vm.flags & DISPLAY_FLAGS_VSYNC_LOW) > + pdata->vidcon1 |= VIDCON1_INV_VSYNC; > + if (vm.flags & DISPLAY_FLAGS_HSYNC_LOW) > + pdata->vidcon1 |= VIDCON1_INV_HSYNC; > + if (vm.flags & DISPLAY_FLAGS_DE_LOW) > + pdata->vidcon1 |= VIDCON1_INV_VDEN; > + if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) > + pdata->vidcon1 |= VIDCON1_INV_VCLK; > } else { > pdata = pdev->dev.platform_data; > if (!pdata) { Ping. Best regards, Tomasz
On 05/19/2013 08:32 PM, Tomasz Figa wrote: > Hi, > > On Wednesday 01 of May 2013 22:00:25 Daniel Vetter wrote: >> On Wed, May 01, 2013 at 09:06:09PM +0200, Tomasz Figa wrote: >>> This patch modifies the driver to perform two stage parsing of video >>> timings from device tree, to get timing information as struct >>> videomode, which contains more data than struct fb_videomode. >>> >>> Thanks to this change, information about polarity of control signals >>> (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard >>> video timings. >>> >>> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> >> Since the drm mode struct also contains flags for sync polarity ... why >> is there no direct of -> drm_mode function? Going through an fb >> videomode in a kms drm driver looks _really_ backwards to me. >> >> Cc'in Dave for the fun of it ;-) > Struct fb_videomode is what exynos_drm_fimd driver uses internally. Sure > it should use drm_mode, but this is not really related to this patch, > because the code added in this patch only fills in the pdata struct, which > for compatibility reasons (the same structure is used for both fbdev and > drm drivers) contains struct fb_videomode. > > OK, now after having a bit of fun, could we merge this patch to at least > have usable support of parallel displays using this driver? I think it's better to use struct display_timings instead of struct fb_videomode in exynos_drm. Thanks.
Hi Joonyoung, On Thursday 06 of June 2013 13:30:49 Joonyoung Shim wrote: > On 05/19/2013 08:32 PM, Tomasz Figa wrote: > > Hi, > > > > On Wednesday 01 of May 2013 22:00:25 Daniel Vetter wrote: > >> On Wed, May 01, 2013 at 09:06:09PM +0200, Tomasz Figa wrote: > >>> This patch modifies the driver to perform two stage parsing of video > >>> timings from device tree, to get timing information as struct > >>> videomode, which contains more data than struct fb_videomode. > >>> > >>> Thanks to this change, information about polarity of control signals > >>> (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard > >>> video timings. > >>> > >>> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> > >> > >> Since the drm mode struct also contains flags for sync polarity ... > >> why > >> is there no direct of -> drm_mode function? Going through an fb > >> videomode in a kms drm driver looks _really_ backwards to me. > >> > >> Cc'in Dave for the fun of it ;-) > > > > Struct fb_videomode is what exynos_drm_fimd driver uses internally. > > Sure it should use drm_mode, but this is not really related to this > > patch, because the code added in this patch only fills in the pdata > > struct, which for compatibility reasons (the same structure is used > > for both fbdev and drm drivers) contains struct fb_videomode. > > > > OK, now after having a bit of fun, could we merge this patch to at > > least have usable support of parallel displays using this driver? > > I think it's better to use struct display_timings instead of struct > fb_videomode in exynos_drm. I agree that fb_videomode struct is a bit unfortunate here, but it seems to be widely used in the exynos_drm_fimd driver. Actually, if I understood it properly, the correct struct to use in DRM drivers is drm_display_mode, but there is no conversion function from struct display_timing to it. IMHO a separate patch introducing such conversion and then another one which modifies the driver to use drm_display_mode everywhere would be the best solution. Best regards, Tomasz
On 06/06/2013 06:47 PM, Tomasz Figa wrote: > Hi Joonyoung, > > On Thursday 06 of June 2013 13:30:49 Joonyoung Shim wrote: >> On 05/19/2013 08:32 PM, Tomasz Figa wrote: >>> Hi, >>> >>> On Wednesday 01 of May 2013 22:00:25 Daniel Vetter wrote: >>>> On Wed, May 01, 2013 at 09:06:09PM +0200, Tomasz Figa wrote: >>>>> This patch modifies the driver to perform two stage parsing of video >>>>> timings from device tree, to get timing information as struct >>>>> videomode, which contains more data than struct fb_videomode. >>>>> >>>>> Thanks to this change, information about polarity of control signals >>>>> (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard >>>>> video timings. >>>>> >>>>> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> >>>> Since the drm mode struct also contains flags for sync polarity ... >>>> why >>>> is there no direct of -> drm_mode function? Going through an fb >>>> videomode in a kms drm driver looks _really_ backwards to me. >>>> >>>> Cc'in Dave for the fun of it ;-) >>> Struct fb_videomode is what exynos_drm_fimd driver uses internally. >>> Sure it should use drm_mode, but this is not really related to this >>> patch, because the code added in this patch only fills in the pdata >>> struct, which for compatibility reasons (the same structure is used >>> for both fbdev and drm drivers) contains struct fb_videomode. >>> >>> OK, now after having a bit of fun, could we merge this patch to at >>> least have usable support of parallel displays using this driver? >> I think it's better to use struct display_timings instead of struct >> fb_videomode in exynos_drm. > I agree that fb_videomode struct is a bit unfortunate here, but it seems > to be widely used in the exynos_drm_fimd driver. > > Actually, if I understood it properly, the correct struct to use in DRM > drivers is drm_display_mode, but there is no conversion function from > struct display_timing to it. It's easy to convert from struct display_timing to videomode and there is conversion function from struct videomode to drm_display_mode. Already it uses at drivers/gpu/drm/tilcdc/tilcdc_panel.c > > IMHO a separate patch introducing such conversion and then another one > which modifies the driver to use drm_display_mode everywhere would be the > best solution. OK, but the struct drm_display_mode doesn't include only timing information so we need also to use struct videomode or struct drm_timing properly.
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index a1669d4..9023efa 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -21,7 +21,9 @@ #include <linux/pm_runtime.h> #include <video/of_display_timing.h> +#include <video/of_videomode.h> #include <video/samsung_fimd.h> +#include <video/videomode.h> #include <drm/exynos_drm.h> #include "exynos_drm_drv.h" @@ -928,18 +930,30 @@ static int fimd_probe(struct platform_device *pdev) DRM_DEBUG_KMS("%s\n", __FILE__); if (pdev->dev.of_node) { + struct videomode vm; + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); if (!pdata) { DRM_ERROR("memory allocation for pdata failed\n"); return -ENOMEM; } - ret = of_get_fb_videomode(dev->of_node, &pdata->panel.timing, - OF_USE_NATIVE_MODE); + ret = of_get_videomode(dev->of_node, &vm, OF_USE_NATIVE_MODE); if (ret) { DRM_ERROR("failed: of_get_fb_videomode() : %d\n", ret); return ret; } + + fb_videomode_from_videomode(&vm, &pdata->panel.timing); + + if (vm.flags & DISPLAY_FLAGS_VSYNC_LOW) + pdata->vidcon1 |= VIDCON1_INV_VSYNC; + if (vm.flags & DISPLAY_FLAGS_HSYNC_LOW) + pdata->vidcon1 |= VIDCON1_INV_HSYNC; + if (vm.flags & DISPLAY_FLAGS_DE_LOW) + pdata->vidcon1 |= VIDCON1_INV_VDEN; + if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) + pdata->vidcon1 |= VIDCON1_INV_VCLK; } else { pdata = pdev->dev.platform_data; if (!pdata) {
This patch modifies the driver to perform two stage parsing of video timings from device tree, to get timing information as struct videomode, which contains more data than struct fb_videomode. Thanks to this change, information about polarity of control signals (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard video timings. Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> --- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)