[PATCH v2 12/12] JUST FOR TEST: Add one-shot trigger to update display
Oh.. understood. Thanks. Mark On 07/03/2015 04:03 PM, Daniel Vetter wrote: > On Fri, Jul 03, 2015 at 03:57:55PM +0800, Mark Zhang wrote: >> On 07/01/2015 10:55 PM, Daniel Vetter wrote: >>> On Wed, Jul 01, 2015 at 08:43:01PM +0800, Mark Zhang wrote: >>>> On 07/01/2015 06:34 PM, Daniel Vetter wrote: >> [...] >>>>> >>>> >>>> Alright, this makes sense. I have no idea about qxl, what I have now is >>>> an ubuntu running on Tegra114. So I'm wondering what I suppose to do is >>>> installing qemu on the ubuntu? >>> >>> My suggestion is just to take the qxl code, move it to the fbdev emulation >>> helper, make it generic and use it. If you want you can do a >>> compile-tested patch on top to switch qxl over to the newly added helpers. >>> No need to install/run qxl itself. Just that qxl seems to have the most >>> complete solution for what you need. >> >> OK, thanks Daniel. I'm not quite familiar with the userspace, but I >> think this is the summary: >> >> - For legacy FB support, we can use the "->dirty" fops, port the qxl >> codes to drm_fb_helper.c, just as you described. > > Note that this is for legacy frontbuffer rendering only. All other screen > updates (setplane, setcrtc, pageflip) will go through the atomic flip > path, thanks to all the atomic helpers. > >> - For atomic page flip, the one-shot can be triggered when flipping the >> window/plane. >> - How about the front buffer rendering/drawing? I mean, if a drm >> userspace app requests a dumb buffer then draws on it, how can we >> trigger the one-shot to update the display? Do we need to add an >> additional IOCTL to do that? > > dumb buffer userspace is required to call dirtyfb ioctl. > -Daniel >
[PATCH v2 12/12] JUST FOR TEST: Add one-shot trigger to update display
On 07/01/2015 10:55 PM, Daniel Vetter wrote: > On Wed, Jul 01, 2015 at 08:43:01PM +0800, Mark Zhang wrote: >> On 07/01/2015 06:34 PM, Daniel Vetter wrote: [...] >>> >> >> Alright, this makes sense. I have no idea about qxl, what I have now is >> an ubuntu running on Tegra114. So I'm wondering what I suppose to do is >> installing qemu on the ubuntu? > > My suggestion is just to take the qxl code, move it to the fbdev emulation > helper, make it generic and use it. If you want you can do a > compile-tested patch on top to switch qxl over to the newly added helpers. > No need to install/run qxl itself. Just that qxl seems to have the most > complete solution for what you need. OK, thanks Daniel. I'm not quite familiar with the userspace, but I think this is the summary: - For legacy FB support, we can use the "->dirty" fops, port the qxl codes to drm_fb_helper.c, just as you described. - For atomic page flip, the one-shot can be triggered when flipping the window/plane. - How about the front buffer rendering/drawing? I mean, if a drm userspace app requests a dumb buffer then draws on it, how can we trigger the one-shot to update the display? Do we need to add an additional IOCTL to do that? Mark > -Daniel >
[PATCH v2 01/12] drm: panel: Add a new private mode flag: DRM_PANEL_FLAG_PREFER_ONE_SHOT
On 07/01/2015 04:54 PM, Daniel Vetter wrote: > On Wed, Jul 01, 2015 at 04:21:44PM +0800, Mark Zhang wrote: >> Normally this flag is set by panel driver so that crtc can enable >> the "one-shot" mode(not scan frames continuously). >> >> Signed-off-by: Mark Zhang >> --- >> include/drm/drm_panel.h |2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h >> index 13ff44b28893..4d51cb380c75 100644 >> --- a/include/drm/drm_panel.h >> +++ b/include/drm/drm_panel.h >> @@ -26,6 +26,8 @@ >> >> #include >> >> +#define DRM_PANEL_FLAG_PREFER_ONE_SHOT (1 << 0) > > This is a panel property, not a mode property. I think it would be much > better to put this directly into the panel struct, or maybe the dsi sink > device stuff or wherever. But mode really doesn't have anything to do how > exactly the pixels get to the panel. Yeah, we've talked about this in patch set v1. I think putting this in drm_display_mode->private_flags already makes things better. If we want to completely remove this in drm_display_mode, I think we need to create some mechanism which panel can use to notify crtc to enable the one-shot mode, or crtc is able to send out a query, if somebody answers, one-shot will be enabled. Mark > -Daniel >
[PATCH v2 12/12] JUST FOR TEST: Add one-shot trigger to update display
On 07/01/2015 04:46 PM, Daniel Vetter wrote: > On Wed, Jul 01, 2015 at 10:36:17AM +0200, Daniel Vetter wrote: >> On Wed, Jul 01, 2015 at 04:21:55PM +0800, Mark Zhang wrote: >>> This HACK adds a workqueue to refresh the display periodically. >>> This is used just for testing. >> >> ->dirty is the drm hook you're looking for, it's meant to flush out any >> frontbuffer rendering. Generic kms clients using the dumb buffers (e.g. >> fedora boot splash) use this already. >> >> And of course you need to upload a new frame every time an (atomic) flip >> happens too, but I guess you have that already. No need at all for a >> periodic upload hack like this. > > btw the nice thing with dirty is that it hands you the exact invalidation > rectangle, which means you can minimize uploads. For atomic flips we plan > to have the same, but it's not implemented yet. > > Another problem is that currently the fbdev helper in drm_fb_helper.c > doesn't support the dirty callback. But there's other drivers which need > this too (e.g. i915 will gain a dirty callback soon) and qxl has all the > code implemented already. So the only thing you need to do is move the qxl > code into drm_fb_helper.c and adapt it to use the dirty callback instead > of directly calling qxl code. Then you should be all set. Note that simply > calling ->dirty from fbdev hooks doesn't work since a lot of those hooks > are called from irq context (cursors and stuff) and hence you need a > workqueue to do the actual dirty call. > I got it. So this is the kernel part I need. Thanks. Mark > Cheers, Daniel >
[PATCH v2 12/12] JUST FOR TEST: Add one-shot trigger to update display
On 07/01/2015 06:34 PM, Daniel Vetter wrote: > On Wed, Jul 01, 2015 at 05:01:52PM +0800, Mark Zhang wrote: >> On 07/01/2015 04:36 PM, Daniel Vetter wrote: >>> On Wed, Jul 01, 2015 at 04:21:55PM +0800, Mark Zhang wrote: >>>> This HACK adds a workqueue to refresh the display periodically. >>>> This is used just for testing. >>> >>> ->dirty is the drm hook you're looking for, it's meant to flush out any >>> frontbuffer rendering. Generic kms clients using the dumb buffers (e.g. >>> fedora boot splash) use this already. >>> >> >> Oh... I did a grep in drm source and are you talking about >> "drm_framebuffer_funcs->dirty"? Yeah, that should work for me.. but that >> requires userspace sending IOCTL to trigger, right? Honestly I'm lazy so >> I created this HACK so that I don't need userspace to test. > > Yeah userspace needs to send ioctl already after each drawing. Generic > userspace does that already since it's required by qxl, udl, soon i915 and > probably a few others too. fbdev emulation is more annyoing but there's > code to move around in these drivers (qxl seems best to me as a starting > point) too. > Alright, this makes sense. I have no idea about qxl, what I have now is an ubuntu running on Tegra114. So I'm wondering what I suppose to do is installing qemu on the ubuntu? Mark > Imo without this you shouldn't merge one-shot, at least not enabled by > default. > -Daniel >
[PATCH v2 08/12] drm: dsi: Add "enter idle" & "exit idle" dcs functions
Oh, yes, if so we need to change most of the functions in drm_mipi_dsi.c.
Mark
On 07/01/2015 05:08 PM, Varka Bhadram wrote:
> On 07/01/2015 01:51 PM, Mark Zhang wrote:
>> Signed-off-by: Mark Zhang
>> ---
>> drivers/gpu/drm/drm_mipi_dsi.c | 36
>> include/drm/drm_mipi_dsi.h |2 ++
>> 2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>> index 2d5ca8eec13a..9bc6ff75eb8f 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -862,6 +862,42 @@ int mipi_dsi_dcs_set_pixel_format(struct
>> mipi_dsi_device *dsi, u8 format)
>> }
>> EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
>>
>> +/**
>> + * mipi_dsi_dcs_enter_idle_mode()
>> + * @dsi: DSI peripheral device
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int mipi_dsi_dcs_enter_idle_mode(struct mipi_dsi_device *dsi)
>> +{
>> +ssize_t err;
>> +
>> +err = mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_IDLE_MODE, NULL, 0);
>> +if (err < 0)
>> +return err;
>> +
>> +return 0;
>> +}
>
> This we can do simply as:
> return mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_IDLE_MODE, NULL, 0);
>
>> +EXPORT_SYMBOL(mipi_dsi_dcs_enter_idle_mode);
>> +
>> +/**
>> + * mipi_dsi_dcs_exit_idle_mode()
>> + * @dsi: DSI peripheral device
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int mipi_dsi_dcs_exit_idle_mode(struct mipi_dsi_device *dsi)
>> +{
>> +ssize_t err;
>> +
>> +err = mipi_dsi_dcs_write(dsi, MIPI_DCS_EXIT_IDLE_MODE, NULL, 0);
>> +if (err < 0)
>> +return err;
>> +
>> +return 0;
>
> For this one also: return mipi_dsi_dcs_write(dsi, MIPI_DCS_EXIT_IDLE_MODE,
> NULL, 0);
>
>
[PATCH v2 10/12] drm/tegra: Suspend dc/dsi/panel in one-shot mode
On 07/01/2015 05:00 PM, Daniel Vetter wrote:
> On Wed, Jul 01, 2015 at 04:21:53PM +0800, Mark Zhang wrote:
>> @@ -756,7 +752,11 @@ tegra_dsi_connector_duplicate_state(struct
>> drm_connector *connector)
>> }
>>
>> static const struct drm_connector_funcs tegra_dsi_connector_funcs = {
>> -.dpms = tegra_dsi_connector_dpms,
>> +/*
>> + * drm_atomic_helper_connector_dpms only handles DPMS ON/OFF,
>> + * so use drm_helper_connector_dpms instead.
>> + */
>> +.dpms = drm_helper_connector_dpms,
>
> Nope, mixing legacy dpms handling into an atomic driver is a no-go. You
> need to use drm_atomic_helper_connector_dpms here.
yeah, so I have to change "drm_atomic_helper_connector_dpms"?
Mark
> -Daniel
>
[PATCH v2 12/12] JUST FOR TEST: Add one-shot trigger to update display
On 07/01/2015 04:36 PM, Daniel Vetter wrote:
> On Wed, Jul 01, 2015 at 04:21:55PM +0800, Mark Zhang wrote:
>> This HACK adds a workqueue to refresh the display periodically.
>> This is used just for testing.
>
> ->dirty is the drm hook you're looking for, it's meant to flush out any
> frontbuffer rendering. Generic kms clients using the dumb buffers (e.g.
> fedora boot splash) use this already.
>
Oh... I did a grep in drm source and are you talking about
"drm_framebuffer_funcs->dirty"? Yeah, that should work for me.. but that
requires userspace sending IOCTL to trigger, right? Honestly I'm lazy so
I created this HACK so that I don't need userspace to test.
> And of course you need to upload a new frame every time an (atomic) flip
> happens too, but I guess you have that already. No need at all for a
> periodic upload hack like this.
>
Yep. Thanks Daniel.
Mark
> Cheers, Daniel
>
>>
>> Signed-off-by: Mark Zhang
>> ---
>> drivers/gpu/drm/tegra/dc.c | 37 +
>> drivers/gpu/drm/tegra/drm.h |1 +
>> 2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>> index 24a91613c4f5..4381691c73f7 100644
>> --- a/drivers/gpu/drm/tegra/dc.c
>> +++ b/drivers/gpu/drm/tegra/dc.c
>> @@ -1296,6 +1296,8 @@ static void tegra_crtc_mode_set_nofb(struct drm_crtc
>> *crtc)
>> value &= ~DISP_CTRL_MODE_MASK;
>> value |= DISP_CTRL_MODE_NC_DISPLAY;
>> tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
>> +
>> +schedule_work(>one_shot_trigger);
>> } else {
>> value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
>> value &= ~DISP_CTRL_MODE_MASK;
>> @@ -1958,6 +1960,40 @@ static void tegra_dc_one_shot_work(struct work_struct
>> *work)
>> drm_modeset_unlock_all(drm);
>> }
>>
>> +static void tegra_dc_one_shot_trigger(struct work_struct *work)
>> +{
>> +struct tegra_dc *dc;
>> +struct drm_connector *connector;
>> +struct drm_device *drm;
>> +unsigned long update_mask = GENERAL_ACT_REQ | NC_HOST_TRIG;
>> +static int first_trigger = 1;
>> +
>> +dc = container_of(work, struct tegra_dc, one_shot_trigger);
>> +drm = dc->base.dev;
>> +msleep(5000);
>> +
>> +if (first_trigger) {
>> +dev_info(dc->dev, "First one-shot triggered.\n");
>> +tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL);
>> +first_trigger = 0;
>> +schedule_work(>one_shot_trigger);
>> +return;
>> +}
>> +
>> +dev_info(dc->dev, "one-shot: Wakeup dc/dsi/panel.\n");
>> +drm_modeset_lock_all(drm);
>> +list_for_each_entry(connector, >mode_config.connector_list, head) {
>> +if (connector->funcs->dpms)
>> +connector->funcs->dpms(connector,
>> +DRM_MODE_DPMS_STANDBY);
>> +}
>> +drm_modeset_unlock_all(drm);
>> +
>> +/* Trigger the one-shot */
>> +tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL);
>> +schedule_work(>one_shot_trigger);
>> +}
>> +
>> static int tegra_dc_probe(struct platform_device *pdev)
>> {
>> unsigned long flags = HOST1X_SYNCPT_CLIENT_MANAGED;
>> @@ -1977,6 +2013,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
>> spin_lock_init(>lock);
>> INIT_LIST_HEAD(>list);
>> INIT_WORK(>one_shot_work, tegra_dc_one_shot_work);
>> +INIT_WORK(>one_shot_trigger, tegra_dc_one_shot_trigger);
>> dc->dev = >dev;
>> dc->soc = id->data;
>>
>> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
>> index 00daf427c831..5d606cacb098 100644
>> --- a/drivers/gpu/drm/tegra/drm.h
>> +++ b/drivers/gpu/drm/tegra/drm.h
>> @@ -132,6 +132,7 @@ struct tegra_dc {
>> struct drm_pending_vblank_event *event;
>>
>> struct work_struct one_shot_work;
>> +struct work_struct one_shot_trigger;
>>
>> const struct tegra_dc_soc_info *soc;
>>
>> --
>> 1.7.9.5
>>
>
[PATCH v2 12/12] JUST FOR TEST: Add one-shot trigger to update display
This HACK adds a workqueue to refresh the display periodically.
This is used just for testing.
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/tegra/dc.c | 37 +
drivers/gpu/drm/tegra/drm.h |1 +
2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 24a91613c4f5..4381691c73f7 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1296,6 +1296,8 @@ static void tegra_crtc_mode_set_nofb(struct drm_crtc
*crtc)
value &= ~DISP_CTRL_MODE_MASK;
value |= DISP_CTRL_MODE_NC_DISPLAY;
tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+
+ schedule_work(>one_shot_trigger);
} else {
value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
value &= ~DISP_CTRL_MODE_MASK;
@@ -1958,6 +1960,40 @@ static void tegra_dc_one_shot_work(struct work_struct
*work)
drm_modeset_unlock_all(drm);
}
+static void tegra_dc_one_shot_trigger(struct work_struct *work)
+{
+ struct tegra_dc *dc;
+ struct drm_connector *connector;
+ struct drm_device *drm;
+ unsigned long update_mask = GENERAL_ACT_REQ | NC_HOST_TRIG;
+ static int first_trigger = 1;
+
+ dc = container_of(work, struct tegra_dc, one_shot_trigger);
+ drm = dc->base.dev;
+ msleep(5000);
+
+ if (first_trigger) {
+ dev_info(dc->dev, "First one-shot triggered.\n");
+ tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL);
+ first_trigger = 0;
+ schedule_work(>one_shot_trigger);
+ return;
+ }
+
+ dev_info(dc->dev, "one-shot: Wakeup dc/dsi/panel.\n");
+ drm_modeset_lock_all(drm);
+ list_for_each_entry(connector, >mode_config.connector_list, head) {
+ if (connector->funcs->dpms)
+ connector->funcs->dpms(connector,
+ DRM_MODE_DPMS_STANDBY);
+ }
+ drm_modeset_unlock_all(drm);
+
+ /* Trigger the one-shot */
+ tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL);
+ schedule_work(>one_shot_trigger);
+}
+
static int tegra_dc_probe(struct platform_device *pdev)
{
unsigned long flags = HOST1X_SYNCPT_CLIENT_MANAGED;
@@ -1977,6 +2013,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
spin_lock_init(>lock);
INIT_LIST_HEAD(>list);
INIT_WORK(>one_shot_work, tegra_dc_one_shot_work);
+ INIT_WORK(>one_shot_trigger, tegra_dc_one_shot_trigger);
dc->dev = >dev;
dc->soc = id->data;
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 00daf427c831..5d606cacb098 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -132,6 +132,7 @@ struct tegra_dc {
struct drm_pending_vblank_event *event;
struct work_struct one_shot_work;
+ struct work_struct one_shot_trigger;
const struct tegra_dc_soc_info *soc;
--
1.7.9.5
[PATCH v2 11/12] drm/tegra: dsi: Set connector DPMS state when enable/disable
This patch fixes a bug when drm_helper_connector_dpms tries to switch connector/encoder/crtc DPMS status. Signed-off-by: Mark Zhang --- drivers/gpu/drm/tegra/dsi.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c index 1d21f149d7f2..7917b7875496 100644 --- a/drivers/gpu/drm/tegra/dsi.c +++ b/drivers/gpu/drm/tegra/dsi.c @@ -898,6 +898,8 @@ static void tegra_dsi_encoder_mode_set(struct drm_encoder *encoder, if (output->panel) drm_panel_enable(output->panel); + output->connector.dpms = DRM_MODE_DPMS_ON; + return; } @@ -937,6 +939,7 @@ static void tegra_dsi_encoder_disable(struct drm_encoder *encoder) tegra_dsi_disable(dsi); + output->connector.dpms = DRM_MODE_DPMS_OFF; return; } -- 1.7.9.5
[PATCH v2 10/12] drm/tegra: Suspend dc/dsi/panel in one-shot mode
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/tegra/dc.c | 58 +
drivers/gpu/drm/tegra/drm.h |3 ++
drivers/gpu/drm/tegra/dsi.c | 76 ---
3 files changed, 132 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 2fdbed9b2b04..24a91613c4f5 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1089,6 +1089,39 @@ static int tegra_dc_wait_idle(struct tegra_dc *dc,
unsigned long timeout)
return -ETIMEDOUT;
}
+static void tegra_dc_dpms(struct drm_crtc *crtc, int mode)
+{
+ struct tegra_dc *dc = to_tegra_dc(crtc);
+ int err;
+ unsigned long value;
+
+ if (mode == DRM_MODE_DPMS_SUSPEND) {
+ tegra_dc_stop(dc);
+ clk_disable_unprepare(dc->clk);
+
+ /*
+* TODO: Powergate dc. This requires we re-init all stuffs
+* next time we want to trigger the one-shot.
+*/
+ }
+
+ if (mode == DRM_MODE_DPMS_STANDBY) {
+ /*
+* TODO: Unpowergate dc if dc is powergated during DPMS SUSPEND
+*/
+ err = clk_prepare_enable(dc->clk);
+ if (err < 0) {
+ dev_err(dc->dev, "failed to enable clock: %d\n", err);
+ return;
+ }
+
+ value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
+ value &= ~DISP_CTRL_MODE_MASK;
+ value |= DISP_CTRL_MODE_NC_DISPLAY;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+ }
+}
+
static void tegra_crtc_disable(struct drm_crtc *crtc)
{
struct tegra_dc *dc = to_tegra_dc(crtc);
@@ -1318,6 +1351,7 @@ static void tegra_crtc_atomic_flush(struct drm_crtc *crtc)
}
static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = {
+ .dpms = tegra_dc_dpms,
.disable = tegra_crtc_disable,
.mode_fixup = tegra_crtc_mode_fixup,
.mode_set_nofb = tegra_crtc_mode_set_nofb,
@@ -1331,6 +1365,7 @@ static const struct drm_crtc_helper_funcs
tegra_crtc_helper_funcs = {
static irqreturn_t tegra_dc_irq(int irq, void *data)
{
struct tegra_dc *dc = data;
+ struct drm_display_mode *mode = >base.state->adjusted_mode;
unsigned long status;
status = tegra_dc_readl(dc, DC_CMD_INT_STATUS);
@@ -1348,6 +1383,9 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
*/
drm_crtc_handle_vblank(>base);
tegra_dc_finish_page_flip(dc);
+
+ if (mode->private_flags & DRM_PANEL_FLAG_PREFER_ONE_SHOT)
+ schedule_work(>one_shot_work);
}
if (status & (WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT)) {
@@ -1901,6 +1939,25 @@ static int tegra_dc_parse_dt(struct tegra_dc *dc)
return 0;
}
+static void tegra_dc_one_shot_work(struct work_struct *work)
+{
+ struct tegra_dc *dc;
+ struct drm_connector *connector;
+ struct drm_device *drm;
+
+ dc = container_of(work, struct tegra_dc, one_shot_work);
+ drm = dc->base.dev;
+
+ dev_dbg(dc->dev, "one-shot: Suspend encoder & connector.\n");
+ drm_modeset_lock_all(drm);
+ list_for_each_entry(connector, >mode_config.connector_list, head) {
+ if (connector->funcs->dpms)
+ connector->funcs->dpms(connector,
+ DRM_MODE_DPMS_SUSPEND);
+ }
+ drm_modeset_unlock_all(drm);
+}
+
static int tegra_dc_probe(struct platform_device *pdev)
{
unsigned long flags = HOST1X_SYNCPT_CLIENT_MANAGED;
@@ -1919,6 +1976,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
spin_lock_init(>lock);
INIT_LIST_HEAD(>list);
+ INIT_WORK(>one_shot_work, tegra_dc_one_shot_work);
dc->dev = >dev;
dc->soc = id->data;
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 659b2fcc986d..00daf427c831 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include
@@ -130,6 +131,8 @@ struct tegra_dc {
/* page-flip handling */
struct drm_pending_vblank_event *event;
+ struct work_struct one_shot_work;
+
const struct tegra_dc_soc_info *soc;
struct iommu_domain *domain;
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index ed970f622903..1d21f149d7f2 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -726,10 +726,6 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
tegra_dsi_soft_reset(dsi->slave);
}
-static void tegra_dsi_connector_dpms(struct drm_connector *connector, int mode)
-{
-}
[PATCH v2 09/12] drm: panel: Add idle/busy in Sharp lq101r1sx01 driver
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 26 +++
1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index e32f1449b067..64eb437ee7b3 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -89,6 +89,18 @@ static __maybe_unused int sharp_panel_read(struct
sharp_panel *sharp,
return err;
}
+static int sharp_panel_idle(struct drm_panel *panel)
+{
+ struct sharp_panel *sharp = to_sharp_panel(panel);
+ int err = 0;
+
+ err = mipi_dsi_dcs_enter_idle_mode(sharp->link1);
+ if (err < 0)
+ dev_err(panel->dev, "failed to enter idle: %d\n", err);
+
+ return err;
+}
+
static int sharp_panel_disable(struct drm_panel *panel)
{
struct sharp_panel *sharp = to_sharp_panel(panel);
@@ -167,6 +179,18 @@ static int sharp_setup_symmetrical_split(struct
mipi_dsi_device *left,
return 0;
}
+static int sharp_panel_busy(struct drm_panel *panel)
+{
+ struct sharp_panel *sharp = to_sharp_panel(panel);
+ int err = 0;
+
+ err = mipi_dsi_dcs_exit_idle_mode(sharp->link1);
+ if (err < 0)
+ dev_err(panel->dev, "failed to exit idle: %d\n", err);
+
+ return err;
+}
+
static int sharp_panel_prepare(struct drm_panel *panel)
{
struct sharp_panel *sharp = to_sharp_panel(panel);
@@ -318,6 +342,8 @@ static int sharp_panel_get_modes(struct drm_panel *panel)
}
static const struct drm_panel_funcs sharp_panel_funcs = {
+ .idle = sharp_panel_idle,
+ .busy = sharp_panel_busy,
.disable = sharp_panel_disable,
.unprepare = sharp_panel_unprepare,
.prepare = sharp_panel_prepare,
--
1.7.9.5
[PATCH v2 08/12] drm: dsi: Add "enter idle" & "exit idle" dcs functions
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/drm_mipi_dsi.c | 36
include/drm/drm_mipi_dsi.h |2 ++
2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 2d5ca8eec13a..9bc6ff75eb8f 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -862,6 +862,42 @@ int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device
*dsi, u8 format)
}
EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
+/**
+ * mipi_dsi_dcs_enter_idle_mode()
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_enter_idle_mode(struct mipi_dsi_device *dsi)
+{
+ ssize_t err;
+
+ err = mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_IDLE_MODE, NULL, 0);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_enter_idle_mode);
+
+/**
+ * mipi_dsi_dcs_exit_idle_mode()
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_exit_idle_mode(struct mipi_dsi_device *dsi)
+{
+ ssize_t err;
+
+ err = mipi_dsi_dcs_write(dsi, MIPI_DCS_EXIT_IDLE_MODE, NULL, 0);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_exit_idle_mode);
+
static int mipi_dsi_drv_probe(struct device *dev)
{
struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index f1d8d0dbb4f1..d949a8ef389f 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -214,6 +214,8 @@ int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi);
int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
enum mipi_dsi_dcs_tear_mode mode);
int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format);
+int mipi_dsi_dcs_enter_idle_mode(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_exit_idle_mode(struct mipi_dsi_device *dsi);
/**
* struct mipi_dsi_driver - DSI driver
--
1.7.9.5
[PATCH v2 07/12] drm/panel: Add panel func: idle/busy
The "idle" function of a drm panel is used to tell panel
there are no more frames coming in and it should remain the
last frame it gets.
Normally this only makes sense for smart panels which has
internal framebuffer.
The "busy" function is opposite to "idle".
Signed-off-by: Mark Zhang
---
include/drm/drm_panel.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index e53f48aa070f..77da292ad2fb 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -70,6 +70,8 @@ struct display_timing;
* the panel. This is the job of the .unprepare() function.
*/
struct drm_panel_funcs {
+ int (*idle)(struct drm_panel *panel);
+ int (*busy)(struct drm_panel *panel);
int (*disable)(struct drm_panel *panel);
int (*unprepare)(struct drm_panel *panel);
int (*prepare)(struct drm_panel *panel);
@@ -89,6 +91,22 @@ struct drm_panel {
struct list_head list;
};
+static inline int drm_panel_idle(struct drm_panel *panel)
+{
+ if (panel && panel->funcs && panel->funcs->idle)
+ return panel->funcs->idle(panel);
+
+ return panel ? -ENOSYS : -EINVAL;
+}
+
+static inline int drm_panel_busy(struct drm_panel *panel)
+{
+ if (panel && panel->funcs && panel->funcs->busy)
+ return panel->funcs->busy(panel);
+
+ return panel ? -ENOSYS : -EINVAL;
+}
+
static inline int drm_panel_unprepare(struct drm_panel *panel)
{
if (panel && panel->funcs && panel->funcs->unprepare)
--
1.7.9.5
[PATCH v2 06/12] drm/tegra: Set NC(Non-contiguous) mode to dc for one-shot
If dc is about to work in one-shot mode, we need to set dc's scan
mode to NC(Non-contiguous).
There are 2 things which can make dc send frame again:
- TE signal is received
- Driver sets the NC_HOST_TRIG_ENABLE
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/tegra/dc.c | 33 +++--
drivers/gpu/drm/tegra/dc.h |5 +
2 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index a287e4fec865..2fdbed9b2b04 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -18,6 +18,7 @@
#include "drm.h"
#include "gem.h"
+#include
#include
#include
#include
@@ -1248,10 +1249,26 @@ static void tegra_crtc_mode_set_nofb(struct drm_crtc
*crtc)
tegra_dc_writel(dc, value, DC_DISP_INTERLACE_CONTROL);
}
- value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
- value &= ~DISP_CTRL_MODE_MASK;
- value |= DISP_CTRL_MODE_C_DISPLAY;
- tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+ if (mode->private_flags & DRM_PANEL_FLAG_PREFER_ONE_SHOT) {
+ /* enable MSF & set MSF polarity */
+ value = MSF_ENABLE | MSF_LSPI;
+ if (mode->private_flags & DRM_PANEL_FLAG_TE_POLARITY_HIGH)
+ value |= MSF_POLARITY_HIGH;
+ else
+ value |= MSF_POLARITY_LOW;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND_OPTION0);
+
+ /* set non-continuous mode */
+ value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
+ value &= ~DISP_CTRL_MODE_MASK;
+ value |= DISP_CTRL_MODE_NC_DISPLAY;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+ } else {
+ value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
+ value &= ~DISP_CTRL_MODE_MASK;
+ value |= DISP_CTRL_MODE_C_DISPLAY;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+ }
value = tegra_dc_readl(dc, DC_CMD_DISPLAY_POWER_CONTROL);
value |= PW0_ENABLE | PW1_ENABLE | PW2_ENABLE | PW3_ENABLE |
@@ -1339,6 +1356,9 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
*/
}
+ if (status & MSF_INT)
+ dev_dbg(dc->dev, "MSF_INT received.\n");
+
return IRQ_HANDLED;
}
@@ -1732,10 +1752,11 @@ static int tegra_dc_init(struct host1x_client *client)
WINDOW_B_THRESHOLD(1) | WINDOW_C_THRESHOLD(1);
tegra_dc_writel(dc, value, DC_DISP_DISP_MEM_HIGH_PRIORITY_TIMER);
- value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT;
+ value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT |
+ WIN_C_UF_INT | MSF_INT;
tegra_dc_writel(dc, value, DC_CMD_INT_ENABLE);
- value = WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT;
+ value = WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT | MSF_INT;
tegra_dc_writel(dc, value, DC_CMD_INT_MASK);
if (dc->soc->supports_border_color)
diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
index 55792daabbb5..4a2d0fec5853 100644
--- a/drivers/gpu/drm/tegra/dc.h
+++ b/drivers/gpu/drm/tegra/dc.h
@@ -27,6 +27,10 @@
#define DC_CMD_CONT_SYNCPT_VSYNC 0x028
#define SYNCPT_VSYNC_ENABLE (1 << 8)
#define DC_CMD_DISPLAY_COMMAND_OPTION0 0x031
+#define MSF_ENABLE (1 << 1)
+#define MSF_LSPI (0 << 2)
+#define MSF_POLARITY_HIGH (0 << 0)
+#define MSF_POLARITY_LOW (1 << 0)
#define DC_CMD_DISPLAY_COMMAND 0x032
#define DISP_CTRL_MODE_STOP (0 << 5)
#define DISP_CTRL_MODE_C_DISPLAY (1 << 5)
@@ -53,6 +57,7 @@
#define WIN_A_UF_INT (1 << 8)
#define WIN_B_UF_INT (1 << 9)
#define WIN_C_UF_INT (1 << 10)
+#define MSF_INT (1 << 12)
#define WIN_A_OF_INT (1 << 14)
#define WIN_B_OF_INT (1 << 15)
#define WIN_C_OF_INT (1 << 16)
--
1.7.9.5
[PATCH v2 05/12] drm: panel: Set TE polarity flag in Sharp lq101r1sx01 driver
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index fd04c419190c..e32f1449b067 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -291,7 +291,8 @@ static const struct drm_display_mode default_mode = {
.vsync_end = 1600 + 4 + 8,
.vtotal = 1600 + 4 + 8 + 32,
.vrefresh = 60,
- .private_flags = DRM_PANEL_FLAG_PREFER_ONE_SHOT,
+ .private_flags = DRM_PANEL_FLAG_PREFER_ONE_SHOT |
+DRM_PANEL_FLAG_TE_POLARITY_LOW,
};
static int sharp_panel_get_modes(struct drm_panel *panel)
--
1.7.9.5
[PATCH v2 04/12] drm: panel: Add DRM panel private mode flag: TE polarity
Add 2 DRM panel private mode flag: TE polarity high/low. Signed-off-by: Mark Zhang --- include/drm/drm_panel.h |2 ++ 1 file changed, 2 insertions(+) diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 4d51cb380c75..e53f48aa070f 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -27,6 +27,8 @@ #include #define DRM_PANEL_FLAG_PREFER_ONE_SHOT (1 << 0) +#define DRM_PANEL_FLAG_TE_POLARITY_HIGH(1 << 1) +#define DRM_PANEL_FLAG_TE_POLARITY_LOW (1 << 2) struct drm_connector; struct drm_device; -- 1.7.9.5
[PATCH v2 03/12] drm: panel: Turn on TE(Tearing Effect) on Sharp lq101r1sx01
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c |7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index da2cf7ab64c2..fd04c419190c 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -238,6 +238,13 @@ static int sharp_panel_prepare(struct drm_panel *panel)
goto poweroff;
}
+ err = mipi_dsi_dcs_set_tear_on(sharp->link1,
+ MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+ if (err < 0) {
+ dev_err(panel->dev, "failed to turn on TE: %d\n", err);
+ goto poweroff;
+ }
+
err = mipi_dsi_dcs_set_display_on(sharp->link1);
if (err < 0) {
dev_err(panel->dev, "failed to set display on: %d\n", err);
--
1.7.9.5
[PATCH v2 02/12] drm: panel: Add one-shot flag to Sharp lq101r1sx01 driver
Sharp lq101r1sx01 has internal framebuffer so it doesn't
require crtc sending frames to it continuously.
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 3cce3ca19601..da2cf7ab64c2 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -284,6 +284,7 @@ static const struct drm_display_mode default_mode = {
.vsync_end = 1600 + 4 + 8,
.vtotal = 1600 + 4 + 8 + 32,
.vrefresh = 60,
+ .private_flags = DRM_PANEL_FLAG_PREFER_ONE_SHOT,
};
static int sharp_panel_get_modes(struct drm_panel *panel)
--
1.7.9.5
[PATCH v2 01/12] drm: panel: Add a new private mode flag: DRM_PANEL_FLAG_PREFER_ONE_SHOT
Normally this flag is set by panel driver so that crtc can enable the "one-shot" mode(not scan frames continuously). Signed-off-by: Mark Zhang --- include/drm/drm_panel.h |2 ++ 1 file changed, 2 insertions(+) diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 13ff44b28893..4d51cb380c75 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -26,6 +26,8 @@ #include +#define DRM_PANEL_FLAG_PREFER_ONE_SHOT (1 << 0) + struct drm_connector; struct drm_device; struct drm_panel; -- 1.7.9.5
[PATCH v2 00/12] Tegra: Add DC one-shot support
This patch set adds the Tegra dc one-shot support. The patch set is tested on Dalmore + Sharp lq101r1sx01. Please be noticed that the patch #12 is not part of the feature, it's just used for testing. Changes in v2: - Define one-shot flag in drm_display_mode->private_flags, according to Daniel's comments - Remove the "te-polarity" property in dts. The TE polarity is determined by panel so there is not necessary to define it in DTS. We can define it in panel driver. - Rebased the patch series on top of 6/29 linux-next. - Fix a bug in DSI suspend DPMS event, to wait DSI idle before suspend - Fix a bug in DSI driver to correct the DPMS state(patch #11) Mark Zhang (12): drm: panel: Add a new private mode flag: DRM_PANEL_FLAG_PREFER_ONE_SHOT drm: panel: Add one-shot flag to Sharp lq101r1sx01 driver drm: panel: Turn on TE(Tearing Effect) on Sharp lq101r1sx01 drm: panel: Add DRM panel private mode flag: TE polarity drm: panel: Set TE polarity flag in Sharp lq101r1sx01 driver drm/tegra: Set NC(Non-contiguous) mode to dc for one-shot drm/panel: Add panel func: idle/busy drm: dsi: Add "enter idle" & "exit idle" dcs functions drm: panel: Add idle/busy in Sharp lq101r1sx01 driver drm/tegra: Suspend dc/dsi/panel in one-shot mode drm/tegra: dsi: Set connector DPMS state when enable/disable JUST FOR TEST: Add one-shot trigger to update display drivers/gpu/drm/drm_mipi_dsi.c | 36 +++ drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 35 +++ drivers/gpu/drm/tegra/dc.c | 128 +-- drivers/gpu/drm/tegra/dc.h |5 + drivers/gpu/drm/tegra/drm.h |4 + drivers/gpu/drm/tegra/dsi.c | 79 +- include/drm/drm_mipi_dsi.h |2 + include/drm/drm_panel.h | 22 8 files changed, 300 insertions(+), 11 deletions(-) -- 1.7.9.5
[RFC PATCH 01/12] drm: Add a new mode flag: DRM_MODE_FLAG_PREFER_ONE_SHOT
I just want to make things easier. If we adding this in panel's meta data, it will be harder to make crtc gets this, since normally encoder talks with panel and crtc talks with encoder. But yes, adding this in panel's metadata makes more sense so if there is a better way to do that, I'm happy to do the changes. Mark From: Daniel Vetter on behalf of Daniel Vetter <dan...@ffwll.ch> Sent: Monday, May 11, 2015 5:27 PM To: Mark Zhang Cc: thierry.reding at gmail.com; linux-tegra at vger.kernel.org; dri-devel at lists.freedesktop.org Subject: Re: [RFC PATCH 01/12] drm: Add a new mode flag: DRM_MODE_FLAG_PREFER_ONE_SHOT On Mon, May 11, 2015 at 09:38:20AM +0800, Mark Zhang wrote: > Normally this flag is set by panel driver so that crtc can enable > the "one-shot" mode(not scan frames continuously). > > Signed-off-by: Mark Zhang > --- > include/uapi/drm/drm_mode.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index dbeba949462a..5447a338e893 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -72,6 +72,7 @@ > #define DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH (6<<14) > #define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM (7<<14) > #define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF (8<<14) > +#define DRM_MODE_FLAG_PREFER_ONE_SHOT(1<<19) tbh this doesn't sound like a mode flag, but something which should be attached to the drm_panel. Especially since all the single-frame modes are highly sink/link specific. Why was this added here instead of to the drm_panel metadata? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[RFC PATCH 12/12] JUST FOR TEST: Add one-shot trigger to update display
This HACK adds a workqueue to refresh the display periodically.
This is used just for testing.
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/tegra/dc.c | 47 +
drivers/gpu/drm/tegra/drm.h | 1 +
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index b8231e2e3c92..48bddc795995 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1262,6 +1262,8 @@ static void tegra_crtc_mode_set_nofb(struct drm_crtc
*crtc)
value &= ~DISP_CTRL_MODE_MASK;
value |= DISP_CTRL_MODE_NC_DISPLAY;
tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+
+ schedule_work(>one_shot_trigger);
} else {
value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
value &= ~DISP_CTRL_MODE_MASK;
@@ -1928,6 +1930,50 @@ static void tegra_dc_one_shot_work(struct work_struct
*work)
*/
}
+static void tegra_dc_one_shot_trigger(struct work_struct *work)
+{
+ struct tegra_dc *dc;
+ struct drm_connector *connector;
+ struct drm_device *drm;
+ unsigned long update_mask = GENERAL_ACT_REQ | NC_HOST_TRIG;
+ static int first_trigger = 1;
+ int err;
+ unsigned long value;
+
+ dc = container_of(work, struct tegra_dc, one_shot_trigger);
+ drm = dc->base.dev;
+ msleep(5000);
+
+ if (first_trigger) {
+ tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL);
+ first_trigger = 0;
+ schedule_work(>one_shot_trigger);
+ return;
+ }
+
+ dev_info(dc->dev, "one-shot: Wakeup dsi/panel.\n");
+ err = clk_prepare_enable(dc->clk);
+ if (err < 0)
+ dev_err(dc->dev, "failed to enable clock: %d\n", err);
+
+ value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
+ value &= ~DISP_CTRL_MODE_MASK;
+ value |= DISP_CTRL_MODE_NC_DISPLAY;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+
+ drm_modeset_lock_all(drm);
+ list_for_each_entry(connector, >mode_config.connector_list, head) {
+ if (connector->funcs->dpms)
+ connector->funcs->dpms(connector,
+ DRM_MODE_DPMS_STANDBY);
+ }
+ drm_modeset_unlock_all(drm);
+
+ /* Trigger the one-shot */
+ tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL);
+ schedule_work(>one_shot_trigger);
+}
+
static int tegra_dc_probe(struct platform_device *pdev)
{
unsigned long flags = HOST1X_SYNCPT_CLIENT_MANAGED;
@@ -1947,6 +1993,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
spin_lock_init(>lock);
INIT_LIST_HEAD(>list);
INIT_WORK(>one_shot_work, tegra_dc_one_shot_work);
+ INIT_WORK(>one_shot_trigger, tegra_dc_one_shot_trigger);
dc->dev = >dev;
dc->soc = id->data;
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 00daf427c831..5d606cacb098 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -132,6 +132,7 @@ struct tegra_dc {
struct drm_pending_vblank_event *event;
struct work_struct one_shot_work;
+ struct work_struct one_shot_trigger;
const struct tegra_dc_soc_info *soc;
--
2.1.4
[RFC PATCH 11/12] drm/tegra: Suspend dc/dsi/panel in one-shot mode
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/tegra/dc.c | 34
drivers/gpu/drm/tegra/drm.h | 3 +++
drivers/gpu/drm/tegra/dsi.c | 63 +
3 files changed, 95 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index b88c29322c6f..b8231e2e3c92 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1330,6 +1330,7 @@ static const struct drm_crtc_helper_funcs
tegra_crtc_helper_funcs = {
static irqreturn_t tegra_dc_irq(int irq, void *data)
{
struct tegra_dc *dc = data;
+ struct drm_display_mode *mode = >base.state->adjusted_mode;
unsigned long status;
status = tegra_dc_readl(dc, DC_CMD_INT_STATUS);
@@ -1342,6 +1343,9 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
dev_info(dc->dev, "%s(): vertical blank\n", __func__);
drm_crtc_handle_vblank(>base);
tegra_dc_finish_page_flip(dc);
+
+ if (mode->flags & DRM_MODE_FLAG_PREFER_ONE_SHOT)
+ schedule_work(>one_shot_work);
}
if (status & (WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT)) {
@@ -1895,6 +1899,35 @@ static int tegra_dc_parse_dt(struct tegra_dc *dc)
return 0;
}
+static void tegra_dc_one_shot_work(struct work_struct *work)
+{
+ struct tegra_dc *dc;
+ struct drm_connector *connector;
+ struct drm_device *drm;
+
+ dc = container_of(work, struct tegra_dc, one_shot_work);
+ drm = dc->base.dev;
+
+ dev_info(dc->dev, "one-shot: Suspend encoder & connector.\n");
+ drm_modeset_lock_all(drm);
+ list_for_each_entry(connector, >mode_config.connector_list, head) {
+ if (connector->funcs->dpms)
+ connector->funcs->dpms(connector,
+ DRM_MODE_DPMS_SUSPEND);
+ }
+ drm_modeset_unlock_all(drm);
+
+ dev_info(dc->dev, "one-shot: Suspend dc.\n");
+ /* Stop dc since dc doesn't have dpms functions */
+ tegra_dc_stop(dc);
+ clk_disable_unprepare(dc->clk);
+
+ /*
+* TODO: Powergate dc. This requires we re-init all stuffs
+* next time we want to trigger the one-shot.
+*/
+}
+
static int tegra_dc_probe(struct platform_device *pdev)
{
unsigned long flags = HOST1X_SYNCPT_CLIENT_MANAGED;
@@ -1913,6 +1946,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
spin_lock_init(>lock);
INIT_LIST_HEAD(>list);
+ INIT_WORK(>one_shot_work, tegra_dc_one_shot_work);
dc->dev = >dev;
dc->soc = id->data;
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 659b2fcc986d..00daf427c831 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include
@@ -130,6 +131,8 @@ struct tegra_dc {
/* page-flip handling */
struct drm_pending_vblank_event *event;
+ struct work_struct one_shot_work;
+
const struct tegra_dc_soc_info *soc;
struct iommu_domain *domain;
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index ed970f622903..7e00279982b7 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -726,10 +726,6 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
tegra_dsi_soft_reset(dsi->slave);
}
-static void tegra_dsi_connector_dpms(struct drm_connector *connector, int mode)
-{
-}
-
static void tegra_dsi_connector_reset(struct drm_connector *connector)
{
struct tegra_dsi_state *state;
@@ -756,7 +752,7 @@ tegra_dsi_connector_duplicate_state(struct drm_connector
*connector)
}
static const struct drm_connector_funcs tegra_dsi_connector_funcs = {
- .dpms = tegra_dsi_connector_dpms,
+ .dpms = drm_helper_connector_dpms,
.reset = tegra_dsi_connector_reset,
.detect = tegra_output_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -784,6 +780,63 @@ static const struct drm_encoder_funcs
tegra_dsi_encoder_funcs = {
static void tegra_dsi_encoder_dpms(struct drm_encoder *encoder, int mode)
{
+ struct tegra_output *output = encoder_to_output(encoder);
+ struct drm_crtc *crtc = encoder->crtc;
+ struct tegra_dc *dc = to_tegra_dc(encoder->crtc);
+ struct tegra_dsi *dsi = to_dsi(output);
+ struct tegra_dsi_state *state;
+ u32 value;
+ int err;
+
+ if (mode == DRM_MODE_DPMS_SUSPEND) {
+ tegra_dsi_video_disable(dsi);
+
+ if (dc) {
+ value = tegra_dc_readl(dc, DC_DISP_DISP_WIN_OPTIONS);
+ value &= ~DSI_ENABLE;
+ tegra_dc_writel(dc, value, DC_DISP_DISP_W
[RFC PATCH 10/12] drm: panel: Add idle/busy in Sharp lq101r1sx01 driver
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 26 +
1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 654575607864..a0a7c80f23d6 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -90,6 +90,18 @@ static __maybe_unused int sharp_panel_read(struct
sharp_panel *sharp,
return err;
}
+static int sharp_panel_idle(struct drm_panel *panel)
+{
+ struct sharp_panel *sharp = to_sharp_panel(panel);
+ int err = 0;
+
+ err = mipi_dsi_dcs_enter_idle_mode(sharp->link1);
+ if (err < 0)
+ dev_err(panel->dev, "failed to enter idle: %d\n", err);
+
+ return err;
+}
+
static int sharp_panel_disable(struct drm_panel *panel)
{
struct sharp_panel *sharp = to_sharp_panel(panel);
@@ -168,6 +180,18 @@ static int sharp_setup_symmetrical_split(struct
mipi_dsi_device *left,
return 0;
}
+static int sharp_panel_busy(struct drm_panel *panel)
+{
+ struct sharp_panel *sharp = to_sharp_panel(panel);
+ int err = 0;
+
+ err = mipi_dsi_dcs_exit_idle_mode(sharp->link1);
+ if (err < 0)
+ dev_err(panel->dev, "failed to exit idle: %d\n", err);
+
+ return err;
+}
+
static int sharp_panel_prepare(struct drm_panel *panel)
{
struct sharp_panel *sharp = to_sharp_panel(panel);
@@ -324,6 +348,8 @@ static int sharp_panel_get_modes(struct drm_panel *panel)
}
static const struct drm_panel_funcs sharp_panel_funcs = {
+ .idle = sharp_panel_idle,
+ .busy = sharp_panel_busy,
.disable = sharp_panel_disable,
.unprepare = sharp_panel_unprepare,
.prepare = sharp_panel_prepare,
--
2.1.4
[RFC PATCH 09/12] drm: dsi: Add "enter idle" & "exit idle" dcs functions
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/drm_mipi_dsi.c | 36
include/drm/drm_mipi_dsi.h | 2 ++
2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 2d5ca8eec13a..9bc6ff75eb8f 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -862,6 +862,42 @@ int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device
*dsi, u8 format)
}
EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
+/**
+ * mipi_dsi_dcs_enter_idle_mode()
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_enter_idle_mode(struct mipi_dsi_device *dsi)
+{
+ ssize_t err;
+
+ err = mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_IDLE_MODE, NULL, 0);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_enter_idle_mode);
+
+/**
+ * mipi_dsi_dcs_exit_idle_mode()
+ * @dsi: DSI peripheral device
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_exit_idle_mode(struct mipi_dsi_device *dsi)
+{
+ ssize_t err;
+
+ err = mipi_dsi_dcs_write(dsi, MIPI_DCS_EXIT_IDLE_MODE, NULL, 0);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_exit_idle_mode);
+
static int mipi_dsi_drv_probe(struct device *dev)
{
struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index f1d8d0dbb4f1..d949a8ef389f 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -214,6 +214,8 @@ int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi);
int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
enum mipi_dsi_dcs_tear_mode mode);
int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format);
+int mipi_dsi_dcs_enter_idle_mode(struct mipi_dsi_device *dsi);
+int mipi_dsi_dcs_exit_idle_mode(struct mipi_dsi_device *dsi);
/**
* struct mipi_dsi_driver - DSI driver
--
2.1.4
[RFC PATCH 08/12] drm/panel: Add panel func: idle/busy
The "idle" function of a drm panel is used to tell panel
there are no more frames coming in and it should remain the
last frame it gets.
Normally this only makes sense for smart panels which has
internal framebuffer.
The "busy" function is opposite to "idle".
Signed-off-by: Mark Zhang
---
include/drm/drm_panel.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 13ff44b28893..0025cd020c40 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -66,6 +66,8 @@ struct display_timing;
* the panel. This is the job of the .unprepare() function.
*/
struct drm_panel_funcs {
+ int (*idle)(struct drm_panel *panel);
+ int (*busy)(struct drm_panel *panel);
int (*disable)(struct drm_panel *panel);
int (*unprepare)(struct drm_panel *panel);
int (*prepare)(struct drm_panel *panel);
@@ -85,6 +87,22 @@ struct drm_panel {
struct list_head list;
};
+static inline int drm_panel_idle(struct drm_panel *panel)
+{
+ if (panel && panel->funcs && panel->funcs->idle)
+ return panel->funcs->idle(panel);
+
+ return panel ? -ENOSYS : -EINVAL;
+}
+
+static inline int drm_panel_busy(struct drm_panel *panel)
+{
+ if (panel && panel->funcs && panel->funcs->busy)
+ return panel->funcs->busy(panel);
+
+ return panel ? -ENOSYS : -EINVAL;
+}
+
static inline int drm_panel_unprepare(struct drm_panel *panel)
{
if (panel && panel->funcs && panel->funcs->unprepare)
--
2.1.4
[RFC PATCH 07/12] drm/tegra: Set NC(Non-contiguous) mode to dc for one-shot
If dc is about to work in one-shot mode, we need to set dc's scan
mode to NC(Non-contiguous).
There are 2 things which can make dc send frame again:
- TE signal is received
- Driver sets the NC_HOST_TRIG_ENABLE
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/tegra/dc.c | 43 +--
drivers/gpu/drm/tegra/dc.h | 5 +
2 files changed, 34 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index a287e4fec865..b88c29322c6f 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1248,10 +1248,26 @@ static void tegra_crtc_mode_set_nofb(struct drm_crtc
*crtc)
tegra_dc_writel(dc, value, DC_DISP_INTERLACE_CONTROL);
}
- value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
- value &= ~DISP_CTRL_MODE_MASK;
- value |= DISP_CTRL_MODE_C_DISPLAY;
- tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+ if (mode->flags & DRM_MODE_FLAG_PREFER_ONE_SHOT) {
+ /* enable MSF & set MSF polarity */
+ value = MSF_ENABLE | MSF_LSPI;
+ if (mode->flags & DRM_MODE_FLAG_TE_POLARITY_HIGH)
+ value |= MSF_POLARITY_HIGH;
+ else
+ value |= MSF_POLARITY_LOW;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND_OPTION0);
+
+ /* set non-continuous mode */
+ value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
+ value &= ~DISP_CTRL_MODE_MASK;
+ value |= DISP_CTRL_MODE_NC_DISPLAY;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+ } else {
+ value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
+ value &= ~DISP_CTRL_MODE_MASK;
+ value |= DISP_CTRL_MODE_C_DISPLAY;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
+ }
value = tegra_dc_readl(dc, DC_CMD_DISPLAY_POWER_CONTROL);
value |= PW0_ENABLE | PW1_ENABLE | PW2_ENABLE | PW3_ENABLE |
@@ -1319,16 +1335,11 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
status = tegra_dc_readl(dc, DC_CMD_INT_STATUS);
tegra_dc_writel(dc, status, DC_CMD_INT_STATUS);
- if (status & FRAME_END_INT) {
- /*
- dev_dbg(dc->dev, "%s(): frame end\n", __func__);
- */
- }
+ if (status & FRAME_END_INT)
+ dev_info(dc->dev, "%s(): frame end\n", __func__);
if (status & VBLANK_INT) {
- /*
- dev_dbg(dc->dev, "%s(): vertical blank\n", __func__);
- */
+ dev_info(dc->dev, "%s(): vertical blank\n", __func__);
drm_crtc_handle_vblank(>base);
tegra_dc_finish_page_flip(dc);
}
@@ -1339,6 +1350,9 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
*/
}
+ if (status & MSF_INT)
+ dev_info(dc->dev, "MSF_INT received.\n");
+
return IRQ_HANDLED;
}
@@ -1732,10 +1746,11 @@ static int tegra_dc_init(struct host1x_client *client)
WINDOW_B_THRESHOLD(1) | WINDOW_C_THRESHOLD(1);
tegra_dc_writel(dc, value, DC_DISP_DISP_MEM_HIGH_PRIORITY_TIMER);
- value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT;
+ value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT |
+ WIN_C_UF_INT | MSF_INT;
tegra_dc_writel(dc, value, DC_CMD_INT_ENABLE);
- value = WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT;
+ value = WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT | MSF_INT;
tegra_dc_writel(dc, value, DC_CMD_INT_MASK);
if (dc->soc->supports_border_color)
diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
index 55792daabbb5..4a2d0fec5853 100644
--- a/drivers/gpu/drm/tegra/dc.h
+++ b/drivers/gpu/drm/tegra/dc.h
@@ -27,6 +27,10 @@
#define DC_CMD_CONT_SYNCPT_VSYNC 0x028
#define SYNCPT_VSYNC_ENABLE (1 << 8)
#define DC_CMD_DISPLAY_COMMAND_OPTION0 0x031
+#define MSF_ENABLE (1 << 1)
+#define MSF_LSPI (0 << 2)
+#define MSF_POLARITY_HIGH (0 << 0)
+#define MSF_POLARITY_LOW (1 << 0)
#define DC_CMD_DISPLAY_COMMAND 0x032
#define DISP_CTRL_MODE_STOP (0 << 5)
#define DISP_CTRL_MODE_C_DISPLAY (1 << 5)
@@ -53,6 +57,7 @@
#define WIN_A_UF_INT (1 << 8)
#define WIN_B_UF_INT (1 << 9)
#define WIN_C_UF_INT (1 << 10)
+#define MSF_INT (1 << 12)
#define WIN_A_OF_INT (1 << 14)
#define WIN_B_OF_INT (1 << 15)
#define WIN_C_OF_INT (1 << 16)
--
2.1.4
[RFC PATCH 06/12] drm: panel: Parse "te-polarity" in Sharp lq101r1sx01 driver
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index b69f88cd15b2..654575607864 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -27,6 +27,7 @@ struct sharp_panel {
struct backlight_device *backlight;
struct regulator *supply;
+ u32 te_polarity;
bool prepared;
bool enabled;
@@ -280,7 +281,7 @@ static int sharp_panel_enable(struct drm_panel *panel)
return 0;
}
-static const struct drm_display_mode default_mode = {
+static struct drm_display_mode default_mode = {
.clock = 278000,
.hdisplay = 2560,
.hsync_start = 2560 + 128,
@@ -297,6 +298,12 @@ static const struct drm_display_mode default_mode = {
static int sharp_panel_get_modes(struct drm_panel *panel)
{
struct drm_display_mode *mode;
+ struct sharp_panel *sharp = to_sharp_panel(panel);
+
+ if (sharp->te_polarity)
+ default_mode.flags |= DRM_MODE_FLAG_TE_POLARITY_HIGH;
+ else
+ default_mode.flags |= DRM_MODE_FLAG_TE_POLARITY_LOW;
mode = drm_mode_duplicate(panel->drm, _mode);
if (!mode) {
@@ -341,6 +348,14 @@ static int sharp_panel_add(struct sharp_panel *sharp)
if (IS_ERR(sharp->supply))
return PTR_ERR(sharp->supply);
+ err = of_property_read_u32(sharp->link1->dev.of_node,
+ "te-polarity", >te_polarity);
+ if (err < 0) {
+ dev_err(>link1->dev,
+ "Read te-polarity failed: %d\n", err);
+ return err;
+ }
+
np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);
if (np) {
sharp->backlight = of_find_backlight_by_node(np);
--
2.1.4
[RFC PATCH 05/12] dt: panel: Add property "te-polarity"
te-polarity indicates the polarity of panel's TE(Tearing Effect) signal. Normally the TE pin is connected to the host SoC. The display controller will send a new frame to panel when the TE signal is triggered. Signed-off-by: Mark Zhang --- Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt | 2 ++ arch/arm/boot/dts/tegra114-dalmore.dts| 2 ++ 2 files changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt index f522bb8e47e1..680ebec9a927 100644 --- a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt +++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt @@ -23,6 +23,7 @@ Required properties (for DSI-LINK1 only): - link2: phandle to the DSI peripheral on the secondary link. Note that the presence of this property marks the containing node as DSI-LINK1. - power-supply: phandle of the regulator that provides the supply voltage +- te-polarity: indicates the TE(Tearing Effect) polarity. 0: Low, 1: High. Optional properties (for DSI-LINK1 only): - backlight: phandle of the backlight device attached to the panel @@ -38,6 +39,7 @@ Example: power-supply = <...>; backlight = <...>; + te-polarity = <0>; }; }; diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts index 8b7aa0dcdc6e..fdb1cc4063a9 100644 --- a/arch/arm/boot/dts/tegra114-dalmore.dts +++ b/arch/arm/boot/dts/tegra114-dalmore.dts @@ -47,6 +47,8 @@ power-supply = <_lcd_reg>; backlight = <>; + + te-polarity = <0>; /* TE_POLARITY_LOW */ }; }; }; -- 2.1.4
[RFC PATCH 04/12] drm: Add DRM mode flag TE polarity
Add 2 DRM mode flag: TE polarity high/low. Signed-off-by: Mark Zhang --- include/uapi/drm/drm_mode.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 5447a338e893..b577cdfb76df 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -73,6 +73,8 @@ #define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM (7<<14) #define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF(8<<14) #define DRM_MODE_FLAG_PREFER_ONE_SHOT (1<<19) +#define DRM_MODE_FLAG_TE_POLARITY_HIGH (1<<20) +#define DRM_MODE_FLAG_TE_POLARITY_LOW (1<<21) /* DPMS flags */ -- 2.1.4
[RFC PATCH 03/12] drm: panel: Turn on TE(Tearing Effect) on Sharp lq101r1sx01
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 19a67d2598c0..b69f88cd15b2 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -238,6 +238,13 @@ static int sharp_panel_prepare(struct drm_panel *panel)
goto poweroff;
}
+ err = mipi_dsi_dcs_set_tear_on(sharp->link1,
+ MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+ if (err < 0) {
+ dev_err(panel->dev, "failed to turn on TE: %d\n", err);
+ goto poweroff;
+ }
+
err = mipi_dsi_dcs_set_display_on(sharp->link1);
if (err < 0) {
dev_err(panel->dev, "failed to set display on: %d\n", err);
--
2.1.4
[RFC PATCH 02/12] drm: panel: Add one-shot flag to Sharp lq101r1sx01 driver
Sharp lq101r1sx01 has internal framebuffer so it doesn't
require crtc sending frames to it continuously.
So set the one-shot flag in the driver.
Signed-off-by: Mark Zhang
---
drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 3cce3ca19601..19a67d2598c0 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -283,6 +283,7 @@ static const struct drm_display_mode default_mode = {
.vsync_start = 1600 + 4,
.vsync_end = 1600 + 4 + 8,
.vtotal = 1600 + 4 + 8 + 32,
+ .flags = DRM_MODE_FLAG_PREFER_ONE_SHOT,
.vrefresh = 60,
};
--
2.1.4
[RFC PATCH 01/12] drm: Add a new mode flag: DRM_MODE_FLAG_PREFER_ONE_SHOT
Normally this flag is set by panel driver so that crtc can enable the "one-shot" mode(not scan frames continuously). Signed-off-by: Mark Zhang --- include/uapi/drm/drm_mode.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index dbeba949462a..5447a338e893 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -72,6 +72,7 @@ #define DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH(6<<14) #define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM (7<<14) #define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF(8<<14) +#define DRM_MODE_FLAG_PREFER_ONE_SHOT (1<<19) /* DPMS flags */ -- 2.1.4
[RFC PATCH 00/12] Tegra: Add DC one-shot support
This patch set adds the Tegra dc one-shot support. The patch set is tested on Dalmore + Sharp lq101r1sx01. Basically this patch set is just for RFC. Some DRM files are changes, I guess maybe there are better solutions for that. Also, Thierry mentioned that "te-polarity" can be designed as a GPIO since not every display controller has a dedicate TE pin like Tegra. So I will try to create v2 after some comments are received. Please be noticed that the patch #12 is not part of the feature, it's just used for testing. Mark Zhang (12): drm: Add a new mode flag: DRM_MODE_FLAG_PREFER_ONE_SHOT drm: panel: Add one-shot flag to Sharp lq101r1sx01 driver drm: panel: Turn on TE(Tearing Effect) on Sharp lq101r1sx01 drm: Add DRM mode flag TE polarity dt: panel: Add property "te-polarity" drm: panel: Parse "te-polarity" in Sharp lq101r1sx01 driver drm/tegra: Set NC(Non-contiguous) mode to dc for one-shot drm/panel: Add panel func: idle/busy drm: dsi: Add "enter idle" & "exit idle" dcs functions drm: panel: Add idle/busy in Sharp lq101r1sx01 driver drm/tegra: Suspend dc/dsi/panel in one-shot mode JUST FOR TEST: Add one-shot trigger to update display .../bindings/panel/sharp,lq101r1sx01.txt | 2 + arch/arm/boot/dts/tegra114-dalmore.dts | 2 + drivers/gpu/drm/drm_mipi_dsi.c | 36 ++ drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c| 51 - drivers/gpu/drm/tegra/dc.c | 124 ++--- drivers/gpu/drm/tegra/dc.h | 5 + drivers/gpu/drm/tegra/drm.h| 4 + drivers/gpu/drm/tegra/dsi.c| 63 ++- include/drm/drm_mipi_dsi.h | 2 + include/drm/drm_panel.h| 18 +++ include/uapi/drm/drm_mode.h| 3 + 11 files changed, 290 insertions(+), 20 deletions(-) -- 2.1.4
[PATCH 5/5] drm/tegra: Add minimal power management
I mean: Reviewed-by: Mark Zhang To make display suspend/resume working, we need some other patches. I will send out the patches soon, based on this patch set. Thanks Thierry. Mark On 01/05/2015 10:50 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, Dec 23, 2014 at 03:32:43PM +0800, Mark Zhang wrote: >> +1 > > Does that translate into a "Reviewed-by" or "Tested-by"? > > Thierry > > * Unknown Key > * 0x7F3EB3A1 >
[PATCH 4/5] gpu: host1x: Provide a proper struct bus_type
Thanks for the explanation. Reviewed-by: Mark Zhang Mark On 01/05/2015 10:49 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, Dec 23, 2014 at 03:30:20PM +0800, Mark Zhang wrote: >> On 12/19/2014 11:24 PM, Thierry Reding wrote: >>> From: Thierry Reding >>> >>> Previously the struct bus_type exported by the host1x infrastructure was >>> only a very basic skeleton. Turn that implementation into a more full- >>> fledged bus to support proper probe ordering and power management. >>> >>> Note that the bus infrastructure needs to be available before any of the >>> drivers can be registered, so the bus needs to be registered before the >>> host1x module. Otherwise drivers could be registered before the module >>> is loaded and trigger a BUG_ON() in driver_register(). To ensure the bus >>> infrastructure is always there, always build the code into the kernel >>> when enabled and register it with a postcore initcall. >>> >> >> So this means there is no chance that host1x can be built as a kernel >> module, right? I'm fine with that, just asking. > > No, it means that not all of host1x can be built as a module. The host1x > bus infrastructure will always be built-in when TEGRA_HOST1X is enabled. > >>> Signed-off-by: Thierry Reding >>> --- >> [...] >>> diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile >>> index c1189f004441..a3e667a1b6f5 100644 >>> --- a/drivers/gpu/host1x/Makefile >>> +++ b/drivers/gpu/host1x/Makefile >>> @@ -1,5 +1,4 @@ >>> host1x-y = \ >>> - bus.o \ >>> syncpt.o \ >>> dev.o \ >>> intr.o \ >>> @@ -13,3 +12,5 @@ host1x-y = \ >>> hw/host1x04.o >>> >>> obj-$(CONFIG_TEGRA_HOST1X) += host1x.o >>> + >>> +obj-y += bus.o >> >> I didn't get it, why we need to do this? > > If CONFIG_TEGRA_HOST1X=m, then obj-$(CONFIG_TEGRA_HOST1X) builds the > bus.o into a module. But we want it to always be built-in. The build > system will descend into the drivers/gpu/host1x directory only if the > TEGRA_HOST1X symbol is selected (either =y or =m), therefore obj-y here > will result in bus.o being built-in whether the rest of host1x is built > as a module or built-in. > >>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c >>> index 0b52f0ea8871..28630a5e9397 100644 >>> --- a/drivers/gpu/host1x/bus.c >>> +++ b/drivers/gpu/host1x/bus.c >>> @@ -72,13 +72,14 @@ static void host1x_subdev_del(struct host1x_subdev >>> *subdev) >> [...] >>> >>> static inline struct host1x_device *to_host1x_device(struct device *dev) >>> >> >> The change looks good to me. Just one thing I feel not comfortable: >> "struct host1x_device" is not a real device, it represents the drm >> device actually. The real tegra host1x device is represented by "struct >> host1x". But the name "host1x_device" makes people confusing, I mean, it >> will make people thinking it's the real "tegra host1x" device then bring >> the reading difficulty. > > The reason behind that name is that host1x provides a bus (real to some > degree, but also virtual). host1x is the device that provides the bus > whereas a host1x_device is a "device" on the "host1x" bus. That's just > like an i2c_client is a "client" on the "I2C" bus. Or an spi_device is a > "device" on the "SPI" bus. > >> Why don't we change this to something like "drm_device" or >> "tegra_drm_device"? > > Other devices can be host1x devices. Some time ago work was being done > on a driver for the CSI/VI hardware (for camera or video input). The > idea was that it would also be instantiated as a host1x_device in some > other subsystem (V4L2 at the time). > > The functionality here is generic and in no way DRM specific. > > Thierry > > * Unknown Key > * 0x7F3EB3A1 >
[PATCH 5/5] drm/tegra: Add minimal power management
+1
Mark
On 12/19/2014 11:24 PM, Thierry Reding wrote:
> From: Thierry Reding
>
> For now only disable the KMS hotplug polling helper logic upon suspend
> and re-enable it on resume.
>
> Signed-off-by: Thierry Reding
> ---
> drivers/gpu/drm/tegra/drm.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 272c2dca3536..16c44b9abbd8 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -889,6 +889,30 @@ static int host1x_drm_remove(struct host1x_device *dev)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int host1x_drm_suspend(struct device *dev)
> +{
> + struct drm_device *drm = dev_get_drvdata(dev);
> +
> + drm_kms_helper_poll_disable(drm);
> +
> + return 0;
> +}
> +
> +static int host1x_drm_resume(struct device *dev)
> +{
> + struct drm_device *drm = dev_get_drvdata(dev);
> +
> + drm_kms_helper_poll_enable(drm);
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops host1x_drm_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(host1x_drm_suspend, host1x_drm_resume)
> +};
> +
> static const struct of_device_id host1x_drm_subdevs[] = {
> { .compatible = "nvidia,tegra20-dc", },
> { .compatible = "nvidia,tegra20-hdmi", },
> @@ -910,6 +934,7 @@ static const struct of_device_id host1x_drm_subdevs[] = {
> static struct host1x_driver host1x_drm_driver = {
> .driver = {
> .name = "drm",
> + .pm = _drm_pm_ops,
> },
> .probe = host1x_drm_probe,
> .remove = host1x_drm_remove,
>
[PATCH 4/5] gpu: host1x: Provide a proper struct bus_type
On 12/19/2014 11:24 PM, Thierry Reding wrote: > From: Thierry Reding > > Previously the struct bus_type exported by the host1x infrastructure was > only a very basic skeleton. Turn that implementation into a more full- > fledged bus to support proper probe ordering and power management. > > Note that the bus infrastructure needs to be available before any of the > drivers can be registered, so the bus needs to be registered before the > host1x module. Otherwise drivers could be registered before the module > is loaded and trigger a BUG_ON() in driver_register(). To ensure the bus > infrastructure is always there, always build the code into the kernel > when enabled and register it with a postcore initcall. > So this means there is no chance that host1x can be built as a kernel module, right? I'm fine with that, just asking. > Signed-off-by: Thierry Reding > --- [...] > diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile > index c1189f004441..a3e667a1b6f5 100644 > --- a/drivers/gpu/host1x/Makefile > +++ b/drivers/gpu/host1x/Makefile > @@ -1,5 +1,4 @@ > host1x-y = \ > - bus.o \ > syncpt.o \ > dev.o \ > intr.o \ > @@ -13,3 +12,5 @@ host1x-y = \ > hw/host1x04.o > > obj-$(CONFIG_TEGRA_HOST1X) += host1x.o > + > +obj-y += bus.o I didn't get it, why we need to do this? > diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c > index 0b52f0ea8871..28630a5e9397 100644 > --- a/drivers/gpu/host1x/bus.c > +++ b/drivers/gpu/host1x/bus.c > @@ -72,13 +72,14 @@ static void host1x_subdev_del(struct host1x_subdev > *subdev) [...] > > static inline struct host1x_device *to_host1x_device(struct device *dev) > The change looks good to me. Just one thing I feel not comfortable: "struct host1x_device" is not a real device, it represents the drm device actually. The real tegra host1x device is represented by "struct host1x". But the name "host1x_device" makes people confusing, I mean, it will make people thinking it's the real "tegra host1x" device then bring the reading difficulty. Why don't we change this to something like "drm_device" or "tegra_drm_device"? Mark
[PATCH] drm/tegra: dsi: Add suspend/resume support
On 12/10/2014 03:29 AM, Sean Paul wrote:
> On Sun, Dec 7, 2014 at 10:40 PM, Mark Zhang wrote:
>> This patch adds the suspend/resume support for Tegra drm
>> driver by calling the corresponding DPMS functions.
[...]
>> + if (dsi->slave) {
>> + err = tegra_dsi_pad_calibrate(dsi->slave);
>> + if (err < 0) {
>> + dev_err(dsi->slave->dev,
>> + "MIPI calibration failed: %d\n", err);
>> + return err;
>> + }
>> + }
>
> Move this duplicate call into tegra_dsi_pad_calibrate() to be
> consistent with the other functions in this file (eg:
> tegra_dsi_set_timeout).
>
Yeah, will do.
>> +
>> err = tegra_dsi_get_muldiv(dsi->format, , );
>> if (err < 0)
>> return err;
>> @@ -833,6 +850,13 @@ static int tegra_output_dsi_setup_clock(struct
>> tegra_output *output,
>> dev_err(dsi->dev, "failed to set parent clock: %d\n", err);
>> return err;
>> }
>> + if (dsi->slave) {
>> + err = tegra_dsi_ganged_setup(dsi);
>> + if (err < 0) {
>> + dev_err(dsi->dev, "DSI ganged setup failed: %d\n",
>> err);
>> + return err;
>> + }
>> + }
>
> I think this belongs in ->enable() instead of here.
>
Actually "tegra_dsi_ganged_setup" is not needed any more. This function
ensures that DSIA & DSIB use the same PLL in ganged mode. But if you
read the Tegra124/Tegra132 TRM, you'll find there is no way to select
the clock source for DSIB. I.E, DSIB always uses the same clock source
with DSIA. Besides, PLLD is the only clock source for DSIA. I.E,
"clk_get_parent"/"clk_set_parent" doesn't make sense for dsia & dsib
clock now.
I've posted a patch(in tegra clock driver) to fix this:
http://article.gmane.org/gmane.linux.ports.tegra/20313
And the corresponding changes in dsi driver will arrive soon.
>>
>> err = clk_set_rate(dsi->clk_parent, plld);
>> if (err < 0) {
[...]
>> +
>> + drm_modeset_lock_all(tegra->drm);
>> + list_for_each_entry(connector,
>> >drm->mode_config.connector_list,
>> + head) {
>> + if (connector->funcs->dpms)
>> + connector->funcs->dpms(connector, connector->dpms);
>> + }
>> + drm_modeset_unlock_all(tegra->drm);
>> +
>> + drm_helper_resume_force_mode(tegra->drm);
>> +
>> + return 0;
>> +}
>> +#endif
>
> So this is the tricky chunk, it definitely does not belong here. I
> think it makes most sense for this to live in drm.c, however
> host1x_driver doesn't have suspend/resume hooks.
>
Agree. drm.c is the best place for putting this.
> I suspect the correct thing to do here is to add them to
> host1x_driver, but I'm not sure how much effort is involved in doing
> that.
>
I thought about this before. If we do it in host1x driver, IIUC, it
means that when host1x starts suspending, it will suspend all host1x
client devices, right? Honestly I feel this is not a good thing despite
I can't tell what's the problem in this right now...
Mark
> Sean
>
>> +
>> +
>> static const struct of_device_id tegra_dsi_of_match[] = {
>> { .compatible = "nvidia,tegra114-dsi", },
>> { },
>> @@ -1557,4 +1636,9 @@ struct platform_driver tegra_dsi_driver = {
>> },
>> .probe = tegra_dsi_probe,
>> .remove = tegra_dsi_remove,
>> +#ifdef CONFIG_PM
>> + .suspend = tegra_dsi_suspend,
>> + .resume = tegra_dsi_resume,
>> +#endif
>> +
>> };
>> --
>> 1.8.1.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drm/tegra: dsi: Add suspend/resume support
This patch adds the suspend/resume support for Tegra drm
driver by calling the corresponding DPMS functions.
Signed-off-by: Mark Zhang
---
Hi,
This patch hooks DSI driver's suspend/resume to implement the whole
display system's suspend/resume. I know this is a super ugly way,
but as we all know, Tegra DRM driver doesn't have a dedicate drm device
so honestly I didn't find a better way to do that.
Thanks,
Mark
drivers/gpu/drm/tegra/dsi.c | 96 ++---
1 file changed, 90 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 33f67fd601c6..25cd0d93f392 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -61,6 +61,9 @@ struct tegra_dsi {
struct tegra_dsi *slave;
};
+static int tegra_dsi_pad_calibrate(struct tegra_dsi *);
+static int tegra_dsi_ganged_setup(struct tegra_dsi *dsi);
+
static inline struct tegra_dsi *
host1x_client_to_dsi(struct host1x_client *client)
{
@@ -805,6 +808,20 @@ static int tegra_output_dsi_setup_clock(struct
tegra_output *output,
lanes = tegra_dsi_get_lanes(dsi);
+ err = tegra_dsi_pad_calibrate(dsi);
+ if (err < 0) {
+ dev_err(dsi->dev, "MIPI calibration failed: %d\n", err);
+ return err;
+ }
+ if (dsi->slave) {
+ err = tegra_dsi_pad_calibrate(dsi->slave);
+ if (err < 0) {
+ dev_err(dsi->slave->dev,
+ "MIPI calibration failed: %d\n", err);
+ return err;
+ }
+ }
+
err = tegra_dsi_get_muldiv(dsi->format, , );
if (err < 0)
return err;
@@ -833,6 +850,13 @@ static int tegra_output_dsi_setup_clock(struct
tegra_output *output,
dev_err(dsi->dev, "failed to set parent clock: %d\n", err);
return err;
}
+ if (dsi->slave) {
+ err = tegra_dsi_ganged_setup(dsi);
+ if (err < 0) {
+ dev_err(dsi->dev, "DSI ganged setup failed: %d\n", err);
+ return err;
+ }
+ }
err = clk_set_rate(dsi->clk_parent, plld);
if (err < 0) {
@@ -1470,12 +1494,6 @@ static int tegra_dsi_probe(struct platform_device *pdev)
goto disable_vdd;
}
- err = tegra_dsi_pad_calibrate(dsi);
- if (err < 0) {
- dev_err(dsi->dev, "MIPI calibration failed: %d\n", err);
- goto mipi_free;
- }
-
dsi->host.ops = _dsi_host_ops;
dsi->host.dev = >dev;
@@ -1544,6 +1562,67 @@ static int tegra_dsi_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM
+static int tegra_dsi_suspend(struct platform_device *pdev, pm_message_t state)
+{
+ struct tegra_dsi *dsi = platform_get_drvdata(pdev);
+ struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent);
+ struct drm_connector *connector;
+
+ if (dsi->master) {
+ regulator_disable(dsi->vdd);
+ return 0;
+ }
+
+ drm_modeset_lock_all(tegra->drm);
+ list_for_each_entry(connector, >drm->mode_config.connector_list,
+ head) {
+ int old_dpms = connector->dpms;
+
+ if (connector->funcs->dpms)
+ connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
+
+ /* Set the old mode back to the connector for resume */
+ connector->dpms = old_dpms;
+ }
+ drm_modeset_unlock_all(tegra->drm);
+
+ regulator_disable(dsi->vdd);
+
+ return 0;
+}
+
+static int tegra_dsi_resume(struct platform_device *pdev)
+{
+ struct tegra_dsi *dsi = platform_get_drvdata(pdev);
+ struct tegra_drm *tegra = dev_get_drvdata(dsi->client.parent);
+ struct drm_connector *connector;
+ int err = 0;
+
+ err = regulator_enable(dsi->vdd);
+ if (err < 0) {
+ dev_err(>dev, "Enable DSI vdd failed: %d\n", err);
+ return err;
+ }
+
+ if (dsi->master)
+ return 0;
+
+ drm_modeset_lock_all(tegra->drm);
+ list_for_each_entry(connector, >drm->mode_config.connector_list,
+ head) {
+ if (connector->funcs->dpms)
+ connector->funcs->dpms(connector, connector->dpms);
+ }
+ drm_modeset_unlock_all(tegra->drm);
+
+ drm_helper_resume_force_mode(tegra->drm);
+
+ return 0;
+}
+#endif
+
+
static const struct of_device_id tegra_dsi_of_match[] = {
{ .compatible = "nvidia,tegra114-dsi", },
{ },
@@ -1557,4 +1636,9 @@ struct platform_driver tegra_dsi_driver = {
},
.probe = tegra_dsi_probe,
.remove = tegra_dsi_remove,
+#ifdef CONFIG_PM
+ .suspend = tegra_dsi_suspend,
+ .resume = tegra_dsi_resume,
+#endif
+
};
--
1.8.1.5
[PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/18/2013 04:40 PM, Mark Zhang wrote: > On 02/18/2013 03:20 PM, Thierry Reding wrote: [...] >> > > Actually the dc which connects with LVDS doesn't know about 1080p > stuffs. All the video mode infos stored in this dc is still 1366x768(I > just checked the register values on my Tegra 30 cardhu). The dc just > scans 1366 pixels then jump to next line(based on the 1080p line stride > we set) and keep scanning. > > So the drm crtc helper(I believe it's function: > drm_crtc_helper_set_config) finds out that video mode is not changed so > a fb_change while not full modeset is triggered. > > I started to test this new series patch set just now. I'll let you know > whether the issue I described still exists. > Tested. This issue still exists. Mark > Mark >> Thierry >>
[PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/18/2013 03:20 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 03:06:10PM +0800, Mark Zhang wrote:
>> On 02/18/2013 02:48 PM, Thierry Reding wrote:
>>> On Mon, Feb 18, 2013 at 02:17:53PM +0800, Mark Zhang wrote:
>>>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>>>> The sequence for replacing the scanout buffer is much shorter than a
>>>>> full mode change operation so implementing this callback considerably
>>>>> speeds up cases where only a new framebuffer is to be scanned out.
>>>>>
>>>>> Signed-off-by: Thierry Reding
>>>>> ---
>>>>> Changes in v3:
>>>>> - split DC_CMD_STATE_CONTROL writes
>>>>>
>>>>> drivers/gpu/drm/tegra/dc.c | 31 +++
>>>>> 1 file changed, 31 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>> index 8f97b1c..cc4c85e 100644
>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>> @@ -114,6 +114,27 @@ static int tegra_dc_add_planes(struct drm_device
>>>>> *drm, struct tegra_dc *dc)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
>>>>> + struct tegra_framebuffer *fb)
>>>>> +{
>>>>> + unsigned long value;
>>>>> +
>>>>> + tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
>>>>> +
>>>>> + value = fb->base.offsets[0] + y * fb->base.pitches[0] +
>>>>> + x * fb->base.bits_per_pixel / 8;
>>>>> +
>>>>> + tegra_dc_writel(dc, fb->obj->paddr + value, DC_WINBUF_START_ADDR);
>>>>> +
>>>>> + value = GENERAL_UPDATE | WIN_A_UPDATE;
>>>>> + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>>>>> +
>>>>> + value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
>>>>> + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>
>>>> Again, what do you think about the "line stride" problem I mentioned:
>>>>
>>>> http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
>>>>
>>>> Don't get me wrong that I also don't want to add a line stride update
>>>> here because that doesn't make sense. It's just a workaround. But we
>>>> need to find a way to make multi-head page flip working.
>>>
>>> I'm not sure that it's something we need to support. .mode_set_base() is
>>> explicitly used only for cases where the framebuffer configuration
>>> doesn't change. That is, only in cases where the only thing that changes
>>> is the physical address of the framebuffer to be displayed.
>>>
>>> The current case where one framebuffer is used as scanout for both
>>> outputs isn't something that page-flipping can support. Page-flipping is
>>> always per-CRTC because typically each CRTC would run at a different
>>> frequency (or even if both ran at the same frequency the VBLANK is very
>>> unlikely to coincide anyway).
>>>
>>> So an application that wants to support page-flipping on two outputs
>>> also needs to take care to set them up with different framebuffers, in
>>> which case what you're describing here can't really happen.
>>
>> Yes, the userspace application needs to setup different framebuffers for
>> different crtcs. So if LVDS & HDMI are connected, here is what the
>> application does:
>>
>> 1. drm reports that 2 connectors: LVDS & HDMI are present in the system
>> 2. The resolution of them are: 1366x768 & 1080p
>> 3. Userspace application allocates 2 fbs according to the resolution got
>> above.
>> 4. call drmModeSetCrtc to switch the fb shown in LVDS to the new fb we
>> created.
>> 5. At this time, remember the line stride setting in dc which connects
>> with LVDS is calculated according to the 1080p resolution. So finally we
>> got a corrupt display because we're showing a 1366x768 fb but with
>> 1080p's line stride.
>
> I don't see how the 1080p stride would still be relevant here. Setting
> the LVDS to the new 1366x768 framebuffer should trigger a full modeset
> and therefore set the stride to the value required by the new 1366x768
> framebuffer.
>
Actually the dc which connects with LVDS doesn't know about 1080p
stuffs. All the video mode infos stored in this dc is still 1366x768(I
just checked the register values on my Tegra 30 cardhu). The dc just
scans 1366 pixels then jump to next line(based on the 1080p line stride
we set) and keep scanning.
So the drm crtc helper(I believe it's function:
drm_crtc_helper_set_config) finds out that video mode is not changed so
a fb_change while not full modeset is triggered.
I started to test this new series patch set just now. I'll let you know
whether the issue I described still exists.
Mark
> Thierry
>
[PATCH v3 0/7] drm/tegra: Miscellaneous enhancements
Hi Thierry, Which version is this patch set based on? I tried to apply it on linux-next 0206 & tot 0215 but failed: Applying: drm: Add consistency check for page-flipping Applying: drm/tegra: Add plane support error: patch failed: drivers/gpu/drm/tegra/drm.h:121 error: drivers/gpu/drm/tegra/drm.h: patch does not apply Patch failed at 0002 drm/tegra: Add plane support When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Mark On 02/14/2013 12:04 AM, Thierry Reding wrote: > This patch series introduces a number of useful features for the Tegra > DRM driver. Patch 1 is a documentation update and adds a warning to the > page-flip IOCTL to catch buggy drivers that return success but failed to > update the crtc->fb field, which would cause the reference counting to > become unbalanced. > > Patch 2 adds support for the additional two planes available on Tegra > hardware, while patch 3 implement .mode_set_base() to allow for some > nice speed up when changing the framebuffer without actually changing > the resolution. Patch 4 adds VBLANK support and patch 5 builds on the > previous two to provide the page-flipping IOCTL. Patch 6 splits the > DC_CMD_STATE_CONTROL register writes into two consecutive writes as > required by the TRM. > > Finally patch 7 adds a new file, named "framebuffers" to debugfs which > can be used to dump a list of framebuffers attached to the DRM device. > This is most useful to inspect the reference count but could also be > helpful in diagnosing out-of-memory conditions and such. > > Thierry > > Thierry Reding (7): > drm: Add consistency check for page-flipping > drm/tegra: Add plane support > drm/tegra: Implement .mode_set_base() > drm/tegra: Implement VBLANK support > drm/tegra: Implement page-flipping support > drm/tegra: Split DC_CMD_STATE_CONTROL register write > drm/tegra: Add list of framebuffers to debugfs > > Documentation/DocBook/drm.tmpl | 6 + > drivers/gpu/drm/drm_crtc.c | 7 + > drivers/gpu/drm/tegra/dc.c | 489 > - > drivers/gpu/drm/tegra/dc.h | 2 +- > drivers/gpu/drm/tegra/drm.c| 103 + > drivers/gpu/drm/tegra/drm.h| 37 > 6 files changed, 533 insertions(+), 111 deletions(-) >
[PATCH v3 2/7] drm/tegra: Add plane support
On 02/18/2013 03:01 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:55:04PM +0800, Mark Zhang wrote:
>> On 02/18/2013 02:40 PM, Thierry Reding wrote:
>>> On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
>>>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>>>> Add support for the B and C planes which support RGB and YUV pixel
>>>>> formats and can be used as overlays or hardware cursor. Currently
>>>>> only 32-bit RGBA pixel formats are advertised.
>>>>>
>>>>> Signed-off-by: Thierry Reding
>>>> [...]
>>>>> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc
>>>>> *dc)
>>>>> +{
>>>>> + unsigned int i;
>>>>> + int err = 0;
>>>>> +
>>>>> + for (i = 0; i < 2; i++) {
>>>>> + struct tegra_plane *plane;
>>>>> +
>>>>> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
>>>>
>>>> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
>>>> disable or crtc disable, we should free "struct tegra_plane" which is
>>>> allocated here. But the memory which devm_kzalloc allocates is only
>>>> freed when the driver detach. This makes lots of memory can't be
>>>> recycled when the plane enable/disable frequently.
>>>
>>> Why would we want to do that? I don't think doing so would even work,
>>> since the planes are resources that need to be registered with the DRM
>>> core and therefore can't be allocated on-demand.
>>>
>>
>> I'm wondering if we add power management codes in the future, the
>> crtc/plane will be disabled/enabled frequently, e.g: system going to
>> suspend state then resume.
>
> In that case it makes even less sense to allocate and deallocate the
> plane every time around. However, any optimizations aside, the allocated
> memory is passed into the core via drm_plane_init(), which registers
> that plane with the given CRTC. So if we deallocate the memory when the
> plane when it is disabled, we'd have to make sure to remove it from the
> core as well. However that would also mean that it disappears from the
> list of planes supported by the CRTC and therefore we wouldn't be able
> to use it again.
>
Alright, I mixed up the "disable" & "remove" of planes and crtcs. You're
right. Just forget what I said in this thread.
Mark
> Thierry
>
[PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/18/2013 02:48 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:17:53PM +0800, Mark Zhang wrote:
>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>> The sequence for replacing the scanout buffer is much shorter than a
>>> full mode change operation so implementing this callback considerably
>>> speeds up cases where only a new framebuffer is to be scanned out.
>>>
>>> Signed-off-by: Thierry Reding
>>> ---
>>> Changes in v3:
>>> - split DC_CMD_STATE_CONTROL writes
>>>
>>> drivers/gpu/drm/tegra/dc.c | 31 +++
>>> 1 file changed, 31 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>> index 8f97b1c..cc4c85e 100644
>>> --- a/drivers/gpu/drm/tegra/dc.c
>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>> @@ -114,6 +114,27 @@ static int tegra_dc_add_planes(struct drm_device *drm,
>>> struct tegra_dc *dc)
>>> return 0;
>>> }
>>>
>>> +static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
>>> +struct tegra_framebuffer *fb)
>>> +{
>>> + unsigned long value;
>>> +
>>> + tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
>>> +
>>> + value = fb->base.offsets[0] + y * fb->base.pitches[0] +
>>> + x * fb->base.bits_per_pixel / 8;
>>> +
>>> + tegra_dc_writel(dc, fb->obj->paddr + value, DC_WINBUF_START_ADDR);
>>> +
>>> + value = GENERAL_UPDATE | WIN_A_UPDATE;
>>> + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>>> +
>>> + value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
>>> + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>>> +
>>> + return 0;
>>> +}
>>
>> Again, what do you think about the "line stride" problem I mentioned:
>>
>> http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
>>
>> Don't get me wrong that I also don't want to add a line stride update
>> here because that doesn't make sense. It's just a workaround. But we
>> need to find a way to make multi-head page flip working.
>
> I'm not sure that it's something we need to support. .mode_set_base() is
> explicitly used only for cases where the framebuffer configuration
> doesn't change. That is, only in cases where the only thing that changes
> is the physical address of the framebuffer to be displayed.
>
> The current case where one framebuffer is used as scanout for both
> outputs isn't something that page-flipping can support. Page-flipping is
> always per-CRTC because typically each CRTC would run at a different
> frequency (or even if both ran at the same frequency the VBLANK is very
> unlikely to coincide anyway).
>
> So an application that wants to support page-flipping on two outputs
> also needs to take care to set them up with different framebuffers, in
> which case what you're describing here can't really happen.
Yes, the userspace application needs to setup different framebuffers for
different crtcs. So if LVDS & HDMI are connected, here is what the
application does:
1. drm reports that 2 connectors: LVDS & HDMI are present in the system
2. The resolution of them are: 1366x768 & 1080p
3. Userspace application allocates 2 fbs according to the resolution got
above.
4. call drmModeSetCrtc to switch the fb shown in LVDS to the new fb we
created.
5. At this time, remember the line stride setting in dc which connects
with LVDS is calculated according to the 1080p resolution. So finally we
got a corrupt display because we're showing a 1366x768 fb but with
1080p's line stride.
Hope I explained this clear. Try this test application if you still have
problems:
https://github.com/dvdhrm/docs/blob/master/drm-howto/modeset-vsync.c
Mark
>
> Thierry
>
[PATCH v3 2/7] drm/tegra: Add plane support
On 02/18/2013 02:40 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>> Add support for the B and C planes which support RGB and YUV pixel
>>> formats and can be used as overlays or hardware cursor. Currently
>>> only 32-bit RGBA pixel formats are advertised.
>>>
>>> Signed-off-by: Thierry Reding
>> [...]
>>> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
>>> +{
>>> + unsigned int i;
>>> + int err = 0;
>>> +
>>> + for (i = 0; i < 2; i++) {
>>> + struct tegra_plane *plane;
>>> +
>>> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
>>
>> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
>> disable or crtc disable, we should free "struct tegra_plane" which is
>> allocated here. But the memory which devm_kzalloc allocates is only
>> freed when the driver detach. This makes lots of memory can't be
>> recycled when the plane enable/disable frequently.
>
> Why would we want to do that? I don't think doing so would even work,
> since the planes are resources that need to be registered with the DRM
> core and therefore can't be allocated on-demand.
>
I'm wondering if we add power management codes in the future, the
crtc/plane will be disabled/enabled frequently, e.g: system going to
suspend state then resume.
> Thierry
>
[PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/18/2013 02:17 PM, Mark Zhang wrote:
> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>> The sequence for replacing the scanout buffer is much shorter than a
>> full mode change operation so implementing this callback considerably
>> speeds up cases where only a new framebuffer is to be scanned out.
>>
>> Signed-off-by: Thierry Reding
>> ---
>> Changes in v3:
>> - split DC_CMD_STATE_CONTROL writes
>>
>> drivers/gpu/drm/tegra/dc.c | 31 +++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>> index 8f97b1c..cc4c85e 100644
>> --- a/drivers/gpu/drm/tegra/dc.c
>> +++ b/drivers/gpu/drm/tegra/dc.c
>> @@ -114,6 +114,27 @@ static int tegra_dc_add_planes(struct drm_device *drm,
>> struct tegra_dc *dc)
>> return 0;
>> }
>>
>> +static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
>> + struct tegra_framebuffer *fb)
>> +{
>> +unsigned long value;
>> +
>> +tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
>> +
>> +value = fb->base.offsets[0] + y * fb->base.pitches[0] +
>> +x * fb->base.bits_per_pixel / 8;
>> +
>> +tegra_dc_writel(dc, fb->obj->paddr + value, DC_WINBUF_START_ADDR);
>> +
>> +value = GENERAL_UPDATE | WIN_A_UPDATE;
>> +tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>> +
>> +value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
>> +tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>> +
>> +return 0;
>> +}
>
> Again, what do you think about the "line stride" problem I mentioned:
>
> http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
>
> Don't get me wrong that I also don't want to add a line stride update
> here because that doesn't make sense. It's just a workaround. But we
> need to find a way to make multi-head page flip working.
Sorry, it's not "multi-head page flip", it should be "multi-head fb
change". For example, if LVDS & HDMI are connected, I can create 4 fbs
for them(every is double-buffered) and call drmModeSetCrtc to switch 2
fbs of LVDS & HDMI to show something.
Mark
>
> Mark
>> +
>> static const struct drm_crtc_funcs tegra_crtc_funcs = {
>> .set_config = drm_crtc_helper_set_config,
>> .destroy = drm_crtc_cleanup,
>> @@ -416,6 +437,15 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
>> return 0;
>> }
>>
>> +static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>> +struct drm_framebuffer *old_fb)
>> +{
>> +struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb);
>> +struct tegra_dc *dc = to_tegra_dc(crtc);
>> +
>> +return tegra_dc_set_base(dc, x, y, fb);
>> +}
>> +
>> static void tegra_crtc_prepare(struct drm_crtc *crtc)
>> {
>> struct tegra_dc *dc = to_tegra_dc(crtc);
>> @@ -495,6 +525,7 @@ static const struct drm_crtc_helper_funcs
>> tegra_crtc_helper_funcs = {
>> .disable = tegra_crtc_disable,
>> .mode_fixup = tegra_crtc_mode_fixup,
>> .mode_set = tegra_crtc_mode_set,
>> +.mode_set_base = tegra_crtc_mode_set_base,
>> .prepare = tegra_crtc_prepare,
>> .commit = tegra_crtc_commit,
>> .load_lut = tegra_crtc_load_lut,
>>
[PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/14/2013 12:05 AM, Thierry Reding wrote:
> The sequence for replacing the scanout buffer is much shorter than a
> full mode change operation so implementing this callback considerably
> speeds up cases where only a new framebuffer is to be scanned out.
>
> Signed-off-by: Thierry Reding
> ---
> Changes in v3:
> - split DC_CMD_STATE_CONTROL writes
>
> drivers/gpu/drm/tegra/dc.c | 31 +++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 8f97b1c..cc4c85e 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -114,6 +114,27 @@ static int tegra_dc_add_planes(struct drm_device *drm,
> struct tegra_dc *dc)
> return 0;
> }
>
> +static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
> + struct tegra_framebuffer *fb)
> +{
> + unsigned long value;
> +
> + tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
> +
> + value = fb->base.offsets[0] + y * fb->base.pitches[0] +
> + x * fb->base.bits_per_pixel / 8;
> +
> + tegra_dc_writel(dc, fb->obj->paddr + value, DC_WINBUF_START_ADDR);
> +
> + value = GENERAL_UPDATE | WIN_A_UPDATE;
> + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
> +
> + value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
> + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
> +
> + return 0;
> +}
Again, what do you think about the "line stride" problem I mentioned:
http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
Don't get me wrong that I also don't want to add a line stride update
here because that doesn't make sense. It's just a workaround. But we
need to find a way to make multi-head page flip working.
Mark
> +
> static const struct drm_crtc_funcs tegra_crtc_funcs = {
> .set_config = drm_crtc_helper_set_config,
> .destroy = drm_crtc_cleanup,
> @@ -416,6 +437,15 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
> return 0;
> }
>
> +static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> + struct drm_framebuffer *old_fb)
> +{
> + struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb);
> + struct tegra_dc *dc = to_tegra_dc(crtc);
> +
> + return tegra_dc_set_base(dc, x, y, fb);
> +}
> +
> static void tegra_crtc_prepare(struct drm_crtc *crtc)
> {
> struct tegra_dc *dc = to_tegra_dc(crtc);
> @@ -495,6 +525,7 @@ static const struct drm_crtc_helper_funcs
> tegra_crtc_helper_funcs = {
> .disable = tegra_crtc_disable,
> .mode_fixup = tegra_crtc_mode_fixup,
> .mode_set = tegra_crtc_mode_set,
> + .mode_set_base = tegra_crtc_mode_set_base,
> .prepare = tegra_crtc_prepare,
> .commit = tegra_crtc_commit,
> .load_lut = tegra_crtc_load_lut,
>
[PATCH v3 2/7] drm/tegra: Add plane support
On 02/14/2013 12:05 AM, Thierry Reding wrote:
> Add support for the B and C planes which support RGB and YUV pixel
> formats and can be used as overlays or hardware cursor. Currently
> only 32-bit RGBA pixel formats are advertised.
>
> Signed-off-by: Thierry Reding
[...]
> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
> +{
> + unsigned int i;
> + int err = 0;
> +
> + for (i = 0; i < 2; i++) {
> + struct tegra_plane *plane;
> +
> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
Using "devm_kzalloc" here seems like not a good idea. Everytime plane
disable or crtc disable, we should free "struct tegra_plane" which is
allocated here. But the memory which devm_kzalloc allocates is only
freed when the driver detach. This makes lots of memory can't be
recycled when the plane enable/disable frequently.
> + if (!plane)
> + return -ENOMEM;
> +
> + plane->index = 1 + i;
> +
> + err = drm_plane_init(drm, >base, 1 << dc->pipe,
> + _plane_funcs, plane_formats,
> + ARRAY_SIZE(plane_formats), false);
> + if (err < 0)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> static const struct drm_crtc_funcs tegra_crtc_funcs = {
> .set_config = drm_crtc_helper_set_config,
> .destroy = drm_crtc_cleanup,
> };
>
> -static void tegra_crtc_dpms(struct drm_crtc *crtc, int mode)
> +static void tegra_crtc_disable(struct drm_crtc *crtc)
> {
> + struct drm_device *drm = crtc->dev;
> + struct drm_plane *plane;
> +
> + list_for_each_entry(plane, >mode_config.plane_list, head) {
> + if (plane->crtc == crtc) {
> + tegra_plane_disable(plane);
> + plane->crtc = NULL;
> +
> + if (plane->fb) {
> + drm_framebuffer_unreference(plane->fb);
> + plane->fb = NULL;
> + }
> + }
> + }
If what I mentioned above(about using devm_kzalloc to allocate "struct
tegra_plane") is correct, we need to free "struct tegra_plane" here.
> }
>
> static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> @@ -46,10 +144,11 @@ static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> return true;
> }
>
[...]
>
Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/18/2013 03:20 PM, Thierry Reding wrote:
On Mon, Feb 18, 2013 at 03:06:10PM +0800, Mark Zhang wrote:
On 02/18/2013 02:48 PM, Thierry Reding wrote:
On Mon, Feb 18, 2013 at 02:17:53PM +0800, Mark Zhang wrote:
On 02/14/2013 12:05 AM, Thierry Reding wrote:
The sequence for replacing the scanout buffer is much shorter than a
full mode change operation so implementing this callback considerably
speeds up cases where only a new framebuffer is to be scanned out.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
Changes in v3:
- split DC_CMD_STATE_CONTROL writes
drivers/gpu/drm/tegra/dc.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 8f97b1c..cc4c85e 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -114,6 +114,27 @@ static int tegra_dc_add_planes(struct drm_device
*drm, struct tegra_dc *dc)
return 0;
}
+static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
+ struct tegra_framebuffer *fb)
+{
+ unsigned long value;
+
+ tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+ value = fb-base.offsets[0] + y * fb-base.pitches[0] +
+ x * fb-base.bits_per_pixel / 8;
+
+ tegra_dc_writel(dc, fb-obj-paddr + value, DC_WINBUF_START_ADDR);
+
+ value = GENERAL_UPDATE | WIN_A_UPDATE;
+ tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+ value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
+ tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+ return 0;
+}
Again, what do you think about the line stride problem I mentioned:
http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
Don't get me wrong that I also don't want to add a line stride update
here because that doesn't make sense. It's just a workaround. But we
need to find a way to make multi-head page flip working.
I'm not sure that it's something we need to support. .mode_set_base() is
explicitly used only for cases where the framebuffer configuration
doesn't change. That is, only in cases where the only thing that changes
is the physical address of the framebuffer to be displayed.
The current case where one framebuffer is used as scanout for both
outputs isn't something that page-flipping can support. Page-flipping is
always per-CRTC because typically each CRTC would run at a different
frequency (or even if both ran at the same frequency the VBLANK is very
unlikely to coincide anyway).
So an application that wants to support page-flipping on two outputs
also needs to take care to set them up with different framebuffers, in
which case what you're describing here can't really happen.
Yes, the userspace application needs to setup different framebuffers for
different crtcs. So if LVDS HDMI are connected, here is what the
application does:
1. drm reports that 2 connectors: LVDS HDMI are present in the system
2. The resolution of them are: 1366x768 1080p
3. Userspace application allocates 2 fbs according to the resolution got
above.
4. call drmModeSetCrtc to switch the fb shown in LVDS to the new fb we
created.
5. At this time, remember the line stride setting in dc which connects
with LVDS is calculated according to the 1080p resolution. So finally we
got a corrupt display because we're showing a 1366x768 fb but with
1080p's line stride.
I don't see how the 1080p stride would still be relevant here. Setting
the LVDS to the new 1366x768 framebuffer should trigger a full modeset
and therefore set the stride to the value required by the new 1366x768
framebuffer.
Actually the dc which connects with LVDS doesn't know about 1080p
stuffs. All the video mode infos stored in this dc is still 1366x768(I
just checked the register values on my Tegra 30 cardhu). The dc just
scans 1366 pixels then jump to next line(based on the 1080p line stride
we set) and keep scanning.
So the drm crtc helper(I believe it's function:
drm_crtc_helper_set_config) finds out that video mode is not changed so
a fb_change while not full modeset is triggered.
I started to test this new series patch set just now. I'll let you know
whether the issue I described still exists.
Mark
Thierry
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/18/2013 04:40 PM, Mark Zhang wrote: On 02/18/2013 03:20 PM, Thierry Reding wrote: [...] Actually the dc which connects with LVDS doesn't know about 1080p stuffs. All the video mode infos stored in this dc is still 1366x768(I just checked the register values on my Tegra 30 cardhu). The dc just scans 1366 pixels then jump to next line(based on the 1080p line stride we set) and keep scanning. So the drm crtc helper(I believe it's function: drm_crtc_helper_set_config) finds out that video mode is not changed so a fb_change while not full modeset is triggered. I started to test this new series patch set just now. I'll let you know whether the issue I described still exists. Tested. This issue still exists. Mark Mark Thierry ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/7] drm/tegra: Add plane support
On 02/14/2013 12:05 AM, Thierry Reding wrote:
Add support for the B and C planes which support RGB and YUV pixel
formats and can be used as overlays or hardware cursor. Currently
only 32-bit RGBA pixel formats are advertised.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
[...]
+static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
+{
+ unsigned int i;
+ int err = 0;
+
+ for (i = 0; i 2; i++) {
+ struct tegra_plane *plane;
+
+ plane = devm_kzalloc(drm-dev, sizeof(*plane), GFP_KERNEL);
Using devm_kzalloc here seems like not a good idea. Everytime plane
disable or crtc disable, we should free struct tegra_plane which is
allocated here. But the memory which devm_kzalloc allocates is only
freed when the driver detach. This makes lots of memory can't be
recycled when the plane enable/disable frequently.
+ if (!plane)
+ return -ENOMEM;
+
+ plane-index = 1 + i;
+
+ err = drm_plane_init(drm, plane-base, 1 dc-pipe,
+ tegra_plane_funcs, plane_formats,
+ ARRAY_SIZE(plane_formats), false);
+ if (err 0)
+ return err;
+ }
+
+ return 0;
+}
+
static const struct drm_crtc_funcs tegra_crtc_funcs = {
.set_config = drm_crtc_helper_set_config,
.destroy = drm_crtc_cleanup,
};
-static void tegra_crtc_dpms(struct drm_crtc *crtc, int mode)
+static void tegra_crtc_disable(struct drm_crtc *crtc)
{
+ struct drm_device *drm = crtc-dev;
+ struct drm_plane *plane;
+
+ list_for_each_entry(plane, drm-mode_config.plane_list, head) {
+ if (plane-crtc == crtc) {
+ tegra_plane_disable(plane);
+ plane-crtc = NULL;
+
+ if (plane-fb) {
+ drm_framebuffer_unreference(plane-fb);
+ plane-fb = NULL;
+ }
+ }
+ }
If what I mentioned above(about using devm_kzalloc to allocate struct
tegra_plane) is correct, we need to free struct tegra_plane here.
}
static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -46,10 +144,11 @@ static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
return true;
}
[...]
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/14/2013 12:05 AM, Thierry Reding wrote:
The sequence for replacing the scanout buffer is much shorter than a
full mode change operation so implementing this callback considerably
speeds up cases where only a new framebuffer is to be scanned out.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
Changes in v3:
- split DC_CMD_STATE_CONTROL writes
drivers/gpu/drm/tegra/dc.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 8f97b1c..cc4c85e 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -114,6 +114,27 @@ static int tegra_dc_add_planes(struct drm_device *drm,
struct tegra_dc *dc)
return 0;
}
+static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
+ struct tegra_framebuffer *fb)
+{
+ unsigned long value;
+
+ tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+ value = fb-base.offsets[0] + y * fb-base.pitches[0] +
+ x * fb-base.bits_per_pixel / 8;
+
+ tegra_dc_writel(dc, fb-obj-paddr + value, DC_WINBUF_START_ADDR);
+
+ value = GENERAL_UPDATE | WIN_A_UPDATE;
+ tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+ value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
+ tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+ return 0;
+}
Again, what do you think about the line stride problem I mentioned:
http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
Don't get me wrong that I also don't want to add a line stride update
here because that doesn't make sense. It's just a workaround. But we
need to find a way to make multi-head page flip working.
Mark
+
static const struct drm_crtc_funcs tegra_crtc_funcs = {
.set_config = drm_crtc_helper_set_config,
.destroy = drm_crtc_cleanup,
@@ -416,6 +437,15 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
return 0;
}
+static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
+ struct drm_framebuffer *old_fb)
+{
+ struct tegra_framebuffer *fb = to_tegra_fb(crtc-fb);
+ struct tegra_dc *dc = to_tegra_dc(crtc);
+
+ return tegra_dc_set_base(dc, x, y, fb);
+}
+
static void tegra_crtc_prepare(struct drm_crtc *crtc)
{
struct tegra_dc *dc = to_tegra_dc(crtc);
@@ -495,6 +525,7 @@ static const struct drm_crtc_helper_funcs
tegra_crtc_helper_funcs = {
.disable = tegra_crtc_disable,
.mode_fixup = tegra_crtc_mode_fixup,
.mode_set = tegra_crtc_mode_set,
+ .mode_set_base = tegra_crtc_mode_set_base,
.prepare = tegra_crtc_prepare,
.commit = tegra_crtc_commit,
.load_lut = tegra_crtc_load_lut,
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/18/2013 02:17 PM, Mark Zhang wrote:
On 02/14/2013 12:05 AM, Thierry Reding wrote:
The sequence for replacing the scanout buffer is much shorter than a
full mode change operation so implementing this callback considerably
speeds up cases where only a new framebuffer is to be scanned out.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
Changes in v3:
- split DC_CMD_STATE_CONTROL writes
drivers/gpu/drm/tegra/dc.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 8f97b1c..cc4c85e 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -114,6 +114,27 @@ static int tegra_dc_add_planes(struct drm_device *drm,
struct tegra_dc *dc)
return 0;
}
+static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
+ struct tegra_framebuffer *fb)
+{
+unsigned long value;
+
+tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+value = fb-base.offsets[0] + y * fb-base.pitches[0] +
+x * fb-base.bits_per_pixel / 8;
+
+tegra_dc_writel(dc, fb-obj-paddr + value, DC_WINBUF_START_ADDR);
+
+value = GENERAL_UPDATE | WIN_A_UPDATE;
+tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
+tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+return 0;
+}
Again, what do you think about the line stride problem I mentioned:
http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
Don't get me wrong that I also don't want to add a line stride update
here because that doesn't make sense. It's just a workaround. But we
need to find a way to make multi-head page flip working.
Sorry, it's not multi-head page flip, it should be multi-head fb
change. For example, if LVDS HDMI are connected, I can create 4 fbs
for them(every is double-buffered) and call drmModeSetCrtc to switch 2
fbs of LVDS HDMI to show something.
Mark
Mark
+
static const struct drm_crtc_funcs tegra_crtc_funcs = {
.set_config = drm_crtc_helper_set_config,
.destroy = drm_crtc_cleanup,
@@ -416,6 +437,15 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
return 0;
}
+static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
+struct drm_framebuffer *old_fb)
+{
+struct tegra_framebuffer *fb = to_tegra_fb(crtc-fb);
+struct tegra_dc *dc = to_tegra_dc(crtc);
+
+return tegra_dc_set_base(dc, x, y, fb);
+}
+
static void tegra_crtc_prepare(struct drm_crtc *crtc)
{
struct tegra_dc *dc = to_tegra_dc(crtc);
@@ -495,6 +525,7 @@ static const struct drm_crtc_helper_funcs
tegra_crtc_helper_funcs = {
.disable = tegra_crtc_disable,
.mode_fixup = tegra_crtc_mode_fixup,
.mode_set = tegra_crtc_mode_set,
+.mode_set_base = tegra_crtc_mode_set_base,
.prepare = tegra_crtc_prepare,
.commit = tegra_crtc_commit,
.load_lut = tegra_crtc_load_lut,
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/7] drm/tegra: Add plane support
On 02/18/2013 02:40 PM, Thierry Reding wrote:
On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
On 02/14/2013 12:05 AM, Thierry Reding wrote:
Add support for the B and C planes which support RGB and YUV pixel
formats and can be used as overlays or hardware cursor. Currently
only 32-bit RGBA pixel formats are advertised.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
[...]
+static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
+{
+ unsigned int i;
+ int err = 0;
+
+ for (i = 0; i 2; i++) {
+ struct tegra_plane *plane;
+
+ plane = devm_kzalloc(drm-dev, sizeof(*plane), GFP_KERNEL);
Using devm_kzalloc here seems like not a good idea. Everytime plane
disable or crtc disable, we should free struct tegra_plane which is
allocated here. But the memory which devm_kzalloc allocates is only
freed when the driver detach. This makes lots of memory can't be
recycled when the plane enable/disable frequently.
Why would we want to do that? I don't think doing so would even work,
since the planes are resources that need to be registered with the DRM
core and therefore can't be allocated on-demand.
I'm wondering if we add power management codes in the future, the
crtc/plane will be disabled/enabled frequently, e.g: system going to
suspend state then resume.
Thierry
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()
On 02/18/2013 02:48 PM, Thierry Reding wrote:
On Mon, Feb 18, 2013 at 02:17:53PM +0800, Mark Zhang wrote:
On 02/14/2013 12:05 AM, Thierry Reding wrote:
The sequence for replacing the scanout buffer is much shorter than a
full mode change operation so implementing this callback considerably
speeds up cases where only a new framebuffer is to be scanned out.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
Changes in v3:
- split DC_CMD_STATE_CONTROL writes
drivers/gpu/drm/tegra/dc.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 8f97b1c..cc4c85e 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -114,6 +114,27 @@ static int tegra_dc_add_planes(struct drm_device *drm,
struct tegra_dc *dc)
return 0;
}
+static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
+struct tegra_framebuffer *fb)
+{
+ unsigned long value;
+
+ tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+ value = fb-base.offsets[0] + y * fb-base.pitches[0] +
+ x * fb-base.bits_per_pixel / 8;
+
+ tegra_dc_writel(dc, fb-obj-paddr + value, DC_WINBUF_START_ADDR);
+
+ value = GENERAL_UPDATE | WIN_A_UPDATE;
+ tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+ value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
+ tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+ return 0;
+}
Again, what do you think about the line stride problem I mentioned:
http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
Don't get me wrong that I also don't want to add a line stride update
here because that doesn't make sense. It's just a workaround. But we
need to find a way to make multi-head page flip working.
I'm not sure that it's something we need to support. .mode_set_base() is
explicitly used only for cases where the framebuffer configuration
doesn't change. That is, only in cases where the only thing that changes
is the physical address of the framebuffer to be displayed.
The current case where one framebuffer is used as scanout for both
outputs isn't something that page-flipping can support. Page-flipping is
always per-CRTC because typically each CRTC would run at a different
frequency (or even if both ran at the same frequency the VBLANK is very
unlikely to coincide anyway).
So an application that wants to support page-flipping on two outputs
also needs to take care to set them up with different framebuffers, in
which case what you're describing here can't really happen.
Yes, the userspace application needs to setup different framebuffers for
different crtcs. So if LVDS HDMI are connected, here is what the
application does:
1. drm reports that 2 connectors: LVDS HDMI are present in the system
2. The resolution of them are: 1366x768 1080p
3. Userspace application allocates 2 fbs according to the resolution got
above.
4. call drmModeSetCrtc to switch the fb shown in LVDS to the new fb we
created.
5. At this time, remember the line stride setting in dc which connects
with LVDS is calculated according to the 1080p resolution. So finally we
got a corrupt display because we're showing a 1366x768 fb but with
1080p's line stride.
Hope I explained this clear. Try this test application if you still have
problems:
https://github.com/dvdhrm/docs/blob/master/drm-howto/modeset-vsync.c
Mark
Thierry
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/7] drm/tegra: Add plane support
On 02/18/2013 03:01 PM, Thierry Reding wrote:
On Mon, Feb 18, 2013 at 02:55:04PM +0800, Mark Zhang wrote:
On 02/18/2013 02:40 PM, Thierry Reding wrote:
On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
On 02/14/2013 12:05 AM, Thierry Reding wrote:
Add support for the B and C planes which support RGB and YUV pixel
formats and can be used as overlays or hardware cursor. Currently
only 32-bit RGBA pixel formats are advertised.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
[...]
+static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc
*dc)
+{
+ unsigned int i;
+ int err = 0;
+
+ for (i = 0; i 2; i++) {
+ struct tegra_plane *plane;
+
+ plane = devm_kzalloc(drm-dev, sizeof(*plane), GFP_KERNEL);
Using devm_kzalloc here seems like not a good idea. Everytime plane
disable or crtc disable, we should free struct tegra_plane which is
allocated here. But the memory which devm_kzalloc allocates is only
freed when the driver detach. This makes lots of memory can't be
recycled when the plane enable/disable frequently.
Why would we want to do that? I don't think doing so would even work,
since the planes are resources that need to be registered with the DRM
core and therefore can't be allocated on-demand.
I'm wondering if we add power management codes in the future, the
crtc/plane will be disabled/enabled frequently, e.g: system going to
suspend state then resume.
In that case it makes even less sense to allocate and deallocate the
plane every time around. However, any optimizations aside, the allocated
memory is passed into the core via drm_plane_init(), which registers
that plane with the given CRTC. So if we deallocate the memory when the
plane when it is disabled, we'd have to make sure to remove it from the
core as well. However that would also mean that it disappears from the
list of planes supported by the CRTC and therefore we wouldn't be able
to use it again.
Alright, I mixed up the disable remove of planes and crtcs. You're
right. Just forget what I said in this thread.
Mark
Thierry
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/5] drm/tegra: Implement VBLANK support
On 01/23/2013 02:21 AM, Jon Mayo wrote: > On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding > wrote: >> Implement support for the VBLANK IOCTL. Note that Tegra is somewhat >> special in this case because it doesn't use the generic IRQ support >> provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one >> interrupt handler for each display controller. >> >> While at it, clean up the way that interrupts are enabled to ensure >> that the VBLANK interrupt only gets enabled when required. >> >> Signed-off-by: Thierry Reding >> --- > > Sorry Thierry, but is this a useful feature? Are applications really > making use of it? Because it means that that an IRQ will have to > trigger every 16.67ms when it is taken, which means the device can't > sleep. (probably OK because it should go back to idle when the app > lets go of the vblank). But worse, for one-shot panels there is no > continuous vblank. I've been doing weird hacks to run a timer when the > smart panel is idle to trick applications into thinking they have a > vblank pulse. But really I think the whole feature of a vblank pulse > is pretty lame and I wish it would die. Were you going to use it for > something specific, or just adding it for completeness? > I guess people don't use this to really wait vblanks because that can be done by polling drm fd. Normally they use this ioctl to get the vblank counts which may be useful for their applications. Mark > - Jon > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Re: [PATCH 4/5] drm/tegra: Implement VBLANK support
On 01/23/2013 02:21 AM, Jon Mayo wrote: On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding thierry.red...@avionic-design.de wrote: Implement support for the VBLANK IOCTL. Note that Tegra is somewhat special in this case because it doesn't use the generic IRQ support provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one interrupt handler for each display controller. While at it, clean up the way that interrupts are enabled to ensure that the VBLANK interrupt only gets enabled when required. Signed-off-by: Thierry Reding thierry.red...@avionic-design.de --- Sorry Thierry, but is this a useful feature? Are applications really making use of it? Because it means that that an IRQ will have to trigger every 16.67ms when it is taken, which means the device can't sleep. (probably OK because it should go back to idle when the app lets go of the vblank). But worse, for one-shot panels there is no continuous vblank. I've been doing weird hacks to run a timer when the smart panel is idle to trick applications into thinking they have a vblank pulse. But really I think the whole feature of a vblank pulse is pretty lame and I wish it would die. Were you going to use it for something specific, or just adding it for completeness? I guess people don't use this to really wait vblanks because that can be done by polling drm fd. Normally they use this ioctl to get the vblank counts which may be useful for their applications. Mark - Jon -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] man: Fix typo and use $() for make expressions
Thanks, David.
After "sudo apt-get install docbook-xsl" and configure the libdrm with
"--enable-manpages", I see the manpages generated.
Mark
On 01/20/2013 05:17 PM, David Herrmann wrote:
> Hi Mark
>
> On Sun, Jan 20, 2013 at 7:39 AM, Mark Zhang wrote:
>> Hi David:
>>
>> Sorry for jumping in. I pulled the ToT libdrm and it seems the manpages
>> will not be built("make html" does nothing), any suggestions?
>
> The manpages should be built automatically if you do "make". Or you
> can try "make -C man" to built the manpages exclusively.
>
> If "make -C man" does nothing, then you don't have the docbook-xsl
> stylesheets on your machine, or xsltproc is missing or you passed
> --disable-manpages to ./configure.
>
> I think the packages for these files are called "xsltproc" and
> "doocbook-xsl" in most distributions.
>
> Regards
> David
>
[PATCH] man: Fix typo and use $() for make expressions
Hi David:
Sorry for jumping in. I pulled the ToT libdrm and it seems the manpages
will not be built("make html" does nothing), any suggestions?
P.S: I've installed xsltproc.
Mark
On 01/19/2013 12:01 AM, David Herrmann wrote:
> On Fri, Jan 18, 2013 at 5:00 PM, David Herrmann
> wrote:
>> Hi Thierry
>>
>> On Fri, Jan 18, 2013 at 1:22 PM, Thierry Reding
>> wrote:
>>> Due to the typo, none of the .xml files would end up in the release
>>> tarball and cause make distcheck as well as builds from the tarball to
>>> fail.
>>>
>>> Using $() isn't strictly necessary but other variables and expressions
>>> use that variant already so it makes the usage consistent.
>>
>> That's weird. "make distcheck" should not be able to build the
>> manpages if the XML files are not available. Also ${} is pretty
>> standard in makefiles, isn't it? I wonder what the problem here is. At
>> least distcheck runs fine on my machine.
>
> Ah sorry, I now saw the "subs" => "subst" typo. Still I wonder why
> distcheck works here. But the patch looks fine. Thanks!
>
>> Regards
>> David
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Re: [PATCH] man: Fix typo and use $() for make expressions
Thanks, David. After sudo apt-get install docbook-xsl and configure the libdrm with --enable-manpages, I see the manpages generated. Mark On 01/20/2013 05:17 PM, David Herrmann wrote: Hi Mark On Sun, Jan 20, 2013 at 7:39 AM, Mark Zhang nvmarkzh...@gmail.com wrote: Hi David: Sorry for jumping in. I pulled the ToT libdrm and it seems the manpages will not be built(make html does nothing), any suggestions? The manpages should be built automatically if you do make. Or you can try make -C man to built the manpages exclusively. If make -C man does nothing, then you don't have the docbook-xsl stylesheets on your machine, or xsltproc is missing or you passed --disable-manpages to ./configure. I think the packages for these files are called xsltproc and doocbook-xsl in most distributions. Regards David ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] man: Fix typo and use $() for make expressions
Hi David:
Sorry for jumping in. I pulled the ToT libdrm and it seems the manpages
will not be built(make html does nothing), any suggestions?
P.S: I've installed xsltproc.
Mark
On 01/19/2013 12:01 AM, David Herrmann wrote:
On Fri, Jan 18, 2013 at 5:00 PM, David Herrmann
dh.herrm...@googlemail.com wrote:
Hi Thierry
On Fri, Jan 18, 2013 at 1:22 PM, Thierry Reding
thierry.red...@avionic-design.de wrote:
Due to the typo, none of the .xml files would end up in the release
tarball and cause make distcheck as well as builds from the tarball to
fail.
Using $() isn't strictly necessary but other variables and expressions
use that variant already so it makes the usage consistent.
That's weird. make distcheck should not be able to build the
manpages if the XML files are not available. Also ${} is pretty
standard in makefiles, isn't it? I wonder what the problem here is. At
least distcheck runs fine on my machine.
Ah sorry, I now saw the subs = subst typo. Still I wonder why
distcheck works here. But the patch looks fine. Thanks!
Regards
David
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/5] drm/tegra: Add plane support
On 01/15/2013 06:50 PM, Lucas Stach wrote:
> Am Dienstag, den 15.01.2013, 17:53 +0800 schrieb Mark Zhang:
>> On 01/15/2013 12:05 AM, Thierry Reding wrote:
>>> Add support for the B and C planes which support RGB and YUV pixel
>>> formats and can be used as overlays or hardware cursor.
>>
>> I think "hardware cursor" has specific meaning for Tegra(e.g: Tegra30
>> has a 32x32 24bpp or 64x64 2bpp hardware cursor). So you may change it
>> to "hardware accelerated cursor"?
>>
> According to the TRM no Tegra has ARGB hardware cursor support, but only
> 2-color. So we talked about doing the hardware cursor by using a plane.
> If the TRM is wrong in this regard and we can get a ARGB cursor on Tegra
> 3 it would be nice to know.
>
Lucas, yes, TRM says "Hardware cursor is supported for 32x32 or for
64x64 2-bpp cursor.", but just as you can see, we can set cursor's
foreground & background color by register "DC_DISP_CURSOR_FOREGROUND_0
" & "DC_DISP_CURSOR_BACKGROUND_0".
So I asked the expert in nvidia and here is the explanation of the
hardware cursor:
"each pixel in the cursor is encoded by 2 bits.
only 3 values are used per pixel: transparent, foreground, background.
when pixel is transparent - no pixel is displayed. (also known as a mask)
when pixel is foreground - color of pixel is 24-bit value in
DC_DISP_CURSOR_FOREGROUND_0.
when pixel is background - color of pixel is 24-bit value in
DC_DISP_CURSOR_BACKGROUND_0.
So I would still phrase it as a 2-bit cursor. It's a palette with 2
colors plus a 1-bit alpha. The palette entries are 24-bit."
Mark
>>>
>>> Signed-off-by: Thierry Reding
>>> ---
>> [...]
>>> +};
>>> +
>>> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
>>> +{
>>> + unsigned int i;
>>> + int err = 0;
>>> +
>>> + for (i = 0; i < 2; i++) {
>>> + struct tegra_plane *plane;
>>> +
>>> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
>>> + if (!plane)
>>> + return -ENOMEM;
>>> +
>>> + plane->index = i;
>>
>> I suggest to change this line to: "plane->index = i + 1;". This makes
>> the plane's index be consistent with Tegra's windows number. And also we
>> don't need to worry about passing "plane->index + 1" to some functions
>> which need to know which window is operating on.
>>
> Again, if we make WIN_C the root window, we can keep the plane index
> assignment as is and get rid of the "index + 1" passing.
>
>
[PATCH v2 5/5] drm/tegra: Implement page-flipping support
On 01/15/2013 12:06 AM, Thierry Reding wrote:
> All the necessary support bits like .mode_set_base() and VBLANK are now
> available, so page-flipping case easily be implemented on top.
>
> Signed-off-by: Thierry Reding
[...]
> +
> +static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer
> *fb,
> + struct drm_pending_vblank_event *event)
> +{
> + struct tegra_framebuffer *newfb = to_tegra_fb(fb);
> + struct tegra_dc *dc = to_tegra_dc(crtc);
> + struct drm_device *drm = crtc->dev;
> + unsigned long flags;
> +
> + if (dc->event)
> + return -EBUSY;
> +
> + tegra_dc_set_base(dc, 0, 0, newfb);
"tegra_dc_set_base" only updates the frame buffer start address to dc
registers. I think this is not enough because it's possible that this
new FB may trigger a full modeset while not just a fb change. For
example, the "bpp" of the new FB differs from current setting in dc
register.
So I suggest to trigger a full modeset here(although it's slower than fb
change) or maybe you can explain why the FB change is enough.
Mark
> +
> + if (event) {
> + event->pipe = dc->pipe;
> +
> + spin_lock_irqsave(>event_lock, flags);
> + dc->event = event;
> + spin_unlock_irqrestore(>event_lock, flags);
> +
> + drm_vblank_get(drm, dc->pipe);
> + }
> +
> + return 0;
> +}
> +
[...]
> struct tegra_output_ops {
> int (*enable)(struct tegra_output *output);
>
[PATCH v2 3/5] drm/tegra: Implement .mode_set_base()
On 01/15/2013 12:05 AM, Thierry Reding wrote:
> The sequence for replacing the scanout buffer is much shorter than a
> full mode change operation so implementing this callback considerably
> speeds up cases where only a new framebuffer is to be scanned out.
>
> Signed-off-by: Thierry Reding
> ---
> drivers/gpu/drm/tegra/dc.c | 29 +
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 157e962..8dd7d8a 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -116,6 +116,25 @@ static int tegra_dc_add_planes(struct drm_device *drm,
> struct tegra_dc *dc)
> return 0;
> }
>
> +static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
> + struct tegra_framebuffer *fb)
> +{
> + unsigned long value;
> +
> + tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
> +
> + value = fb->base.offsets[0] + y * fb->base.pitches[0] +
> + x * fb->base.bits_per_pixel / 8;
> +
> + tegra_dc_writel(dc, fb->obj->paddr + value, DC_WINBUF_START_ADDR);
> +
Add one line here:
tegra_dc_writel(dc, fb->base.pitches[0], DC_WIN_LINE_STRIDE);
I mentioned in previous mail that the page-flip doesn't work well while
multiple heads attached(in my test case, LVDS & HDMI are enabled). And I
found out that this is because we didn't update the line stride
according to the new FB.
But ideally, I think we don't need to update the line stride setting.
The reason that we need it here is: We set a "incorrect" line stride
intentionally when multiple-head is enabled.
I use a Tegra30 cardhu board for testing. The LVDS panel works on
1366x768 while the resolution of the HDMI is 1080p. The size of the FB
created during KMS is 1080p. To make the LVDS & HDMI show correct, we
set the dc0's(connected with LVDS) line stride to 7680(1920x4). So the
final result is, the LVDS panel shows the (0,0)/(1366,768) portion in a
1080p FB.
But the video mode of the LVDS which reported to user space is still
1366x768. So the userspace program creates 2 FBs based on that and ask
drm driver to flip them. So we need to update the line stride here.
Actually, I think we need to find a better solution when multi-head is
enabled. For example, create a large FB and make two dcs display the
different part of it(just like the XRANDR's extension mode). Or maybe we
can take a look at other drm drivers for inspiration.
Mark
> + value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
> + value |= GENERAL_UPDATE | WIN_A_UPDATE;
> + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
> +
> + return 0;
> +}
> +
> static const struct drm_crtc_funcs tegra_crtc_funcs = {
> .set_config = drm_crtc_helper_set_config,
> .destroy = drm_crtc_cleanup,
> @@ -404,6 +423,15 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
> return 0;
> }
>
> +static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> + struct drm_framebuffer *old_fb)
> +{
> + struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb);
> + struct tegra_dc *dc = to_tegra_dc(crtc);
> +
> + return tegra_dc_set_base(dc, x, y, fb);
> +}
> +
> static void tegra_crtc_prepare(struct drm_crtc *crtc)
> {
> struct tegra_dc *dc = to_tegra_dc(crtc);
> @@ -483,6 +511,7 @@ static const struct drm_crtc_helper_funcs
> tegra_crtc_helper_funcs = {
> .dpms = tegra_crtc_dpms,
> .mode_fixup = tegra_crtc_mode_fixup,
> .mode_set = tegra_crtc_mode_set,
> + .mode_set_base = tegra_crtc_mode_set_base,
> .prepare = tegra_crtc_prepare,
> .commit = tegra_crtc_commit,
> .load_lut = tegra_crtc_load_lut,
>
[PATCH v2 5/5] drm/tegra: Implement page-flipping support
On 01/16/2013 07:53 PM, Lucas Stach wrote: > Am Mittwoch, den 16.01.2013, 19:10 +0800 schrieb Mark Zhang: >> I'm testing the pageflip & vblank change on cardhu. Seems the HDMI >> doesn't work(LVDS only is OK). I'll let you know if I get something. >> > Just to provide another data point: I'm running this series and don't > see any failures with DVI output. Though I'm only run a single output, > not some dual-head config. > Yes. HDMI only is OK. I met some problems when the LVDS & HDMI are enabled at the same time. Mark > Even vbltest from libdrm/tests works and shows correct refresh rate. > > Regards, > Lucas >
Re: [PATCH v2 2/5] drm/tegra: Add plane support
On 01/15/2013 06:50 PM, Lucas Stach wrote:
Am Dienstag, den 15.01.2013, 17:53 +0800 schrieb Mark Zhang:
On 01/15/2013 12:05 AM, Thierry Reding wrote:
Add support for the B and C planes which support RGB and YUV pixel
formats and can be used as overlays or hardware cursor.
I think hardware cursor has specific meaning for Tegra(e.g: Tegra30
has a 32x32 24bpp or 64x64 2bpp hardware cursor). So you may change it
to hardware accelerated cursor?
According to the TRM no Tegra has ARGB hardware cursor support, but only
2-color. So we talked about doing the hardware cursor by using a plane.
If the TRM is wrong in this regard and we can get a ARGB cursor on Tegra
3 it would be nice to know.
Lucas, yes, TRM says Hardware cursor is supported for 32x32 or for
64x64 2-bpp cursor., but just as you can see, we can set cursor's
foreground background color by register DC_DISP_CURSOR_FOREGROUND_0
DC_DISP_CURSOR_BACKGROUND_0.
So I asked the expert in nvidia and here is the explanation of the
hardware cursor:
each pixel in the cursor is encoded by 2 bits.
only 3 values are used per pixel: transparent, foreground, background.
when pixel is transparent - no pixel is displayed. (also known as a mask)
when pixel is foreground - color of pixel is 24-bit value in
DC_DISP_CURSOR_FOREGROUND_0.
when pixel is background - color of pixel is 24-bit value in
DC_DISP_CURSOR_BACKGROUND_0.
So I would still phrase it as a 2-bit cursor. It's a palette with 2
colors plus a 1-bit alpha. The palette entries are 24-bit.
Mark
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
[...]
+};
+
+static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
+{
+ unsigned int i;
+ int err = 0;
+
+ for (i = 0; i 2; i++) {
+ struct tegra_plane *plane;
+
+ plane = devm_kzalloc(drm-dev, sizeof(*plane), GFP_KERNEL);
+ if (!plane)
+ return -ENOMEM;
+
+ plane-index = i;
I suggest to change this line to: plane-index = i + 1;. This makes
the plane's index be consistent with Tegra's windows number. And also we
don't need to worry about passing plane-index + 1 to some functions
which need to know which window is operating on.
Again, if we make WIN_C the root window, we can keep the plane index
assignment as is and get rid of the index + 1 passing.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 5/5] drm/tegra: Implement page-flipping support
I'm testing the pageflip & vblank change on cardhu. Seems the HDMI
doesn't work(LVDS only is OK). I'll let you know if I get something.
Mark
On 01/15/2013 12:06 AM, Thierry Reding wrote:
> All the necessary support bits like .mode_set_base() and VBLANK are now
> available, so page-flipping case easily be implemented on top.
>
> Signed-off-by: Thierry Reding
> ---
> drivers/gpu/drm/tegra/dc.c | 72
> +
> drivers/gpu/drm/tegra/drm.c | 9 ++
> drivers/gpu/drm/tegra/drm.h | 5
> 3 files changed, 86 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 3aa7ccc..ce4e14a 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -161,7 +161,78 @@ void tegra_dc_disable_vblank(struct tegra_dc *dc)
> spin_unlock_irqrestore(>lock, flags);
> }
>
> +static void tegra_dc_finish_page_flip(struct tegra_dc *dc)
> +{
> + struct drm_pending_vblank_event *event;
> + struct drm_device *drm = dc->base.dev;
> + unsigned long flags;
> + struct timeval now;
> +
> + spin_lock_irqsave(>event_lock, flags);
> + event = dc->event;
> + dc->event = NULL;
> + spin_unlock_irqrestore(>event_lock, flags);
> +
> + if (!event)
> + return;
> +
> + event->event.sequence = drm_vblank_count_and_time(drm, dc->pipe, );
> + event->event.tv_sec = now.tv_sec;
> + event->event.tv_usec = now.tv_usec;
> +
> + spin_lock_irqsave(>event_lock, flags);
> + list_add_tail(>base.link, >base.file_priv->event_list);
> + wake_up_interruptible(>base.file_priv->event_wait);
> + spin_unlock_irqrestore(>event_lock, flags);
> +
> + drm_vblank_put(drm, dc->pipe);
> +}
> +
> +void tegra_dc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file)
> +{
> + struct tegra_dc *dc = to_tegra_dc(crtc);
> + struct drm_device *drm = crtc->dev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>event_lock, flags);
> +
> + if (dc->event && dc->event->base.file_priv == file) {
> + dc->event->base.destroy(>event->base);
> + drm_vblank_put(drm, dc->pipe);
> + dc->event = NULL;
> + }
> +
> + spin_unlock_irqrestore(>event_lock, flags);
> +}
> +
> +static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer
> *fb,
> + struct drm_pending_vblank_event *event)
> +{
> + struct tegra_framebuffer *newfb = to_tegra_fb(fb);
> + struct tegra_dc *dc = to_tegra_dc(crtc);
> + struct drm_device *drm = crtc->dev;
> + unsigned long flags;
> +
> + if (dc->event)
> + return -EBUSY;
> +
> + tegra_dc_set_base(dc, 0, 0, newfb);
> +
> + if (event) {
> + event->pipe = dc->pipe;
> +
> + spin_lock_irqsave(>event_lock, flags);
> + dc->event = event;
> + spin_unlock_irqrestore(>event_lock, flags);
> +
> + drm_vblank_get(drm, dc->pipe);
> + }
> +
> + return 0;
> +}
> +
> static const struct drm_crtc_funcs tegra_crtc_funcs = {
> + .page_flip = tegra_dc_page_flip,
> .set_config = drm_crtc_helper_set_config,
> .destroy = drm_crtc_cleanup,
> };
> @@ -556,6 +627,7 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
> dev_dbg(dc->dev, "%s(): vertical blank\n", __func__);
> */
> drm_handle_vblank(dc->base.dev, dc->pipe);
> + tegra_dc_finish_page_flip(dc);
> }
>
> if (status & (WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT)) {
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 62f8121..7bab784 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -129,11 +129,20 @@ static void tegra_drm_disable_vblank(struct drm_device
> *drm, int pipe)
> tegra_dc_disable_vblank(dc);
> }
>
> +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
> +{
> + struct drm_crtc *crtc;
> +
> + list_for_each_entry(crtc, >mode_config.crtc_list, head)
> + tegra_dc_cancel_page_flip(crtc, file);
> +}
> +
> struct drm_driver tegra_drm_driver = {
> .driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM,
> .load = tegra_drm_load,
> .unload = tegra_drm_unload,
> .open = tegra_drm_open,
> + .preclose = tegra_drm_preclose,
> .lastclose = tegra_drm_lastclose,
>
> .get_vblank_counter = drm_vblank_count,
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index ca742b2..1f1cb37 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -100,6 +100,9 @@ struct tegra_dc {
> struct drm_info_list *debugfs_files;
> struct drm_minor *minor;
> struct dentry *debugfs;
> +
> + /* page-flip handling */
> + struct drm_pending_vblank_event *event;
> };
>
> static inline struct tegra_dc
Re: [PATCH v2 5/5] drm/tegra: Implement page-flipping support
I'm testing the pageflip vblank change on cardhu. Seems the HDMI
doesn't work(LVDS only is OK). I'll let you know if I get something.
Mark
On 01/15/2013 12:06 AM, Thierry Reding wrote:
All the necessary support bits like .mode_set_base() and VBLANK are now
available, so page-flipping case easily be implemented on top.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
drivers/gpu/drm/tegra/dc.c | 72
+
drivers/gpu/drm/tegra/drm.c | 9 ++
drivers/gpu/drm/tegra/drm.h | 5
3 files changed, 86 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 3aa7ccc..ce4e14a 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -161,7 +161,78 @@ void tegra_dc_disable_vblank(struct tegra_dc *dc)
spin_unlock_irqrestore(dc-lock, flags);
}
+static void tegra_dc_finish_page_flip(struct tegra_dc *dc)
+{
+ struct drm_pending_vblank_event *event;
+ struct drm_device *drm = dc-base.dev;
+ unsigned long flags;
+ struct timeval now;
+
+ spin_lock_irqsave(drm-event_lock, flags);
+ event = dc-event;
+ dc-event = NULL;
+ spin_unlock_irqrestore(drm-event_lock, flags);
+
+ if (!event)
+ return;
+
+ event-event.sequence = drm_vblank_count_and_time(drm, dc-pipe, now);
+ event-event.tv_sec = now.tv_sec;
+ event-event.tv_usec = now.tv_usec;
+
+ spin_lock_irqsave(drm-event_lock, flags);
+ list_add_tail(event-base.link, event-base.file_priv-event_list);
+ wake_up_interruptible(event-base.file_priv-event_wait);
+ spin_unlock_irqrestore(drm-event_lock, flags);
+
+ drm_vblank_put(drm, dc-pipe);
+}
+
+void tegra_dc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file)
+{
+ struct tegra_dc *dc = to_tegra_dc(crtc);
+ struct drm_device *drm = crtc-dev;
+ unsigned long flags;
+
+ spin_lock_irqsave(drm-event_lock, flags);
+
+ if (dc-event dc-event-base.file_priv == file) {
+ dc-event-base.destroy(dc-event-base);
+ drm_vblank_put(drm, dc-pipe);
+ dc-event = NULL;
+ }
+
+ spin_unlock_irqrestore(drm-event_lock, flags);
+}
+
+static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer
*fb,
+ struct drm_pending_vblank_event *event)
+{
+ struct tegra_framebuffer *newfb = to_tegra_fb(fb);
+ struct tegra_dc *dc = to_tegra_dc(crtc);
+ struct drm_device *drm = crtc-dev;
+ unsigned long flags;
+
+ if (dc-event)
+ return -EBUSY;
+
+ tegra_dc_set_base(dc, 0, 0, newfb);
+
+ if (event) {
+ event-pipe = dc-pipe;
+
+ spin_lock_irqsave(drm-event_lock, flags);
+ dc-event = event;
+ spin_unlock_irqrestore(drm-event_lock, flags);
+
+ drm_vblank_get(drm, dc-pipe);
+ }
+
+ return 0;
+}
+
static const struct drm_crtc_funcs tegra_crtc_funcs = {
+ .page_flip = tegra_dc_page_flip,
.set_config = drm_crtc_helper_set_config,
.destroy = drm_crtc_cleanup,
};
@@ -556,6 +627,7 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
dev_dbg(dc-dev, %s(): vertical blank\n, __func__);
*/
drm_handle_vblank(dc-base.dev, dc-pipe);
+ tegra_dc_finish_page_flip(dc);
}
if (status (WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT)) {
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 62f8121..7bab784 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -129,11 +129,20 @@ static void tegra_drm_disable_vblank(struct drm_device
*drm, int pipe)
tegra_dc_disable_vblank(dc);
}
+static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
+{
+ struct drm_crtc *crtc;
+
+ list_for_each_entry(crtc, drm-mode_config.crtc_list, head)
+ tegra_dc_cancel_page_flip(crtc, file);
+}
+
struct drm_driver tegra_drm_driver = {
.driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM,
.load = tegra_drm_load,
.unload = tegra_drm_unload,
.open = tegra_drm_open,
+ .preclose = tegra_drm_preclose,
.lastclose = tegra_drm_lastclose,
.get_vblank_counter = drm_vblank_count,
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index ca742b2..1f1cb37 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -100,6 +100,9 @@ struct tegra_dc {
struct drm_info_list *debugfs_files;
struct drm_minor *minor;
struct dentry *debugfs;
+
+ /* page-flip handling */
+ struct drm_pending_vblank_event *event;
};
static inline struct tegra_dc *host1x_client_to_dc(struct host1x_client
*client)
@@ -149,6 +152,8 @@ extern int tegra_dc_setup_window(struct
Re: [PATCH v2 5/5] drm/tegra: Implement page-flipping support
On 01/16/2013 07:53 PM, Lucas Stach wrote: Am Mittwoch, den 16.01.2013, 19:10 +0800 schrieb Mark Zhang: I'm testing the pageflip vblank change on cardhu. Seems the HDMI doesn't work(LVDS only is OK). I'll let you know if I get something. Just to provide another data point: I'm running this series and don't see any failures with DVI output. Though I'm only run a single output, not some dual-head config. Yes. HDMI only is OK. I met some problems when the LVDS HDMI are enabled at the same time. Mark Even vbltest from libdrm/tests works and shows correct refresh rate. Regards, Lucas ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/5] drm/tegra: Implement .mode_set_base()
On 01/15/2013 12:05 AM, Thierry Reding wrote:
The sequence for replacing the scanout buffer is much shorter than a
full mode change operation so implementing this callback considerably
speeds up cases where only a new framebuffer is to be scanned out.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
drivers/gpu/drm/tegra/dc.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 157e962..8dd7d8a 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -116,6 +116,25 @@ static int tegra_dc_add_planes(struct drm_device *drm,
struct tegra_dc *dc)
return 0;
}
+static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
+ struct tegra_framebuffer *fb)
+{
+ unsigned long value;
+
+ tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+ value = fb-base.offsets[0] + y * fb-base.pitches[0] +
+ x * fb-base.bits_per_pixel / 8;
+
+ tegra_dc_writel(dc, fb-obj-paddr + value, DC_WINBUF_START_ADDR);
+
Add one line here:
tegra_dc_writel(dc, fb-base.pitches[0], DC_WIN_LINE_STRIDE);
I mentioned in previous mail that the page-flip doesn't work well while
multiple heads attached(in my test case, LVDS HDMI are enabled). And I
found out that this is because we didn't update the line stride
according to the new FB.
But ideally, I think we don't need to update the line stride setting.
The reason that we need it here is: We set a incorrect line stride
intentionally when multiple-head is enabled.
I use a Tegra30 cardhu board for testing. The LVDS panel works on
1366x768 while the resolution of the HDMI is 1080p. The size of the FB
created during KMS is 1080p. To make the LVDS HDMI show correct, we
set the dc0's(connected with LVDS) line stride to 7680(1920x4). So the
final result is, the LVDS panel shows the (0,0)/(1366,768) portion in a
1080p FB.
But the video mode of the LVDS which reported to user space is still
1366x768. So the userspace program creates 2 FBs based on that and ask
drm driver to flip them. So we need to update the line stride here.
Actually, I think we need to find a better solution when multi-head is
enabled. For example, create a large FB and make two dcs display the
different part of it(just like the XRANDR's extension mode). Or maybe we
can take a look at other drm drivers for inspiration.
Mark
+ value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
+ value |= GENERAL_UPDATE | WIN_A_UPDATE;
+ tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+ return 0;
+}
+
static const struct drm_crtc_funcs tegra_crtc_funcs = {
.set_config = drm_crtc_helper_set_config,
.destroy = drm_crtc_cleanup,
@@ -404,6 +423,15 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
return 0;
}
+static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
+ struct drm_framebuffer *old_fb)
+{
+ struct tegra_framebuffer *fb = to_tegra_fb(crtc-fb);
+ struct tegra_dc *dc = to_tegra_dc(crtc);
+
+ return tegra_dc_set_base(dc, x, y, fb);
+}
+
static void tegra_crtc_prepare(struct drm_crtc *crtc)
{
struct tegra_dc *dc = to_tegra_dc(crtc);
@@ -483,6 +511,7 @@ static const struct drm_crtc_helper_funcs
tegra_crtc_helper_funcs = {
.dpms = tegra_crtc_dpms,
.mode_fixup = tegra_crtc_mode_fixup,
.mode_set = tegra_crtc_mode_set,
+ .mode_set_base = tegra_crtc_mode_set_base,
.prepare = tegra_crtc_prepare,
.commit = tegra_crtc_commit,
.load_lut = tegra_crtc_load_lut,
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/5] drm/tegra: Implement page-flipping support
On 01/15/2013 12:06 AM, Thierry Reding wrote:
All the necessary support bits like .mode_set_base() and VBLANK are now
available, so page-flipping case easily be implemented on top.
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
[...]
+
+static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer
*fb,
+ struct drm_pending_vblank_event *event)
+{
+ struct tegra_framebuffer *newfb = to_tegra_fb(fb);
+ struct tegra_dc *dc = to_tegra_dc(crtc);
+ struct drm_device *drm = crtc-dev;
+ unsigned long flags;
+
+ if (dc-event)
+ return -EBUSY;
+
+ tegra_dc_set_base(dc, 0, 0, newfb);
tegra_dc_set_base only updates the frame buffer start address to dc
registers. I think this is not enough because it's possible that this
new FB may trigger a full modeset while not just a fb change. For
example, the bpp of the new FB differs from current setting in dc
register.
So I suggest to trigger a full modeset here(although it's slower than fb
change) or maybe you can explain why the FB change is enough.
Mark
+
+ if (event) {
+ event-pipe = dc-pipe;
+
+ spin_lock_irqsave(drm-event_lock, flags);
+ dc-event = event;
+ spin_unlock_irqrestore(drm-event_lock, flags);
+
+ drm_vblank_get(drm, dc-pipe);
+ }
+
+ return 0;
+}
+
[...]
struct tegra_output_ops {
int (*enable)(struct tegra_output *output);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/5] drm/tegra: Add plane support
On 01/15/2013 12:05 AM, Thierry Reding wrote:
> Add support for the B and C planes which support RGB and YUV pixel
> formats and can be used as overlays or hardware cursor.
I think "hardware cursor" has specific meaning for Tegra(e.g: Tegra30
has a 32x32 24bpp or 64x64 2bpp hardware cursor). So you may change it
to "hardware accelerated cursor"?
>
> Signed-off-by: Thierry Reding
> ---
[...]
> +
> +static const uint32_t plane_formats[] = {
> + DRM_FORMAT_XRGB,
> + DRM_FORMAT_YUV422,
I haven't found something related with YUV format in this patch set. For
example, "tegra_dc_format" also doesn't take YUV into consideration. So
remove this line.
> +};
> +
> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
> +{
> + unsigned int i;
> + int err = 0;
> +
> + for (i = 0; i < 2; i++) {
> + struct tegra_plane *plane;
> +
> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
> + if (!plane)
> + return -ENOMEM;
> +
> + plane->index = i;
I suggest to change this line to: "plane->index = i + 1;". This makes
the plane's index be consistent with Tegra's windows number. And also we
don't need to worry about passing "plane->index + 1" to some functions
which need to know which window is operating on.
> +
> + err = drm_plane_init(drm, >base, 1 << dc->pipe,
> + _plane_funcs, plane_formats,
> + ARRAY_SIZE(plane_formats), false);
> + if (err < 0)
> + return err;
> + }
> +
> + return 0;
> +}
> +
[...]
>
> +int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
> + const struct tegra_dc_window *window)
> +{
> + unsigned h_offset, v_offset, h_size, v_size, h_dda, v_dda, bpp;
> + unsigned long value;
> +
> + bpp = window->bits_per_pixel / 8;
> +
> + value = WINDOW_A_SELECT << index;
> + tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER);
> +
> + tegra_dc_writel(dc, window->format, DC_WIN_COLOR_DEPTH);
> + tegra_dc_writel(dc, 0, DC_WIN_BYTE_SWAP);
> +
> + value = V_POSITION(window->dst.y) | H_POSITION(window->dst.x);
> + tegra_dc_writel(dc, value, DC_WIN_POSITION);
> +
> + value = V_SIZE(window->dst.h) | H_SIZE(window->dst.w);
> + tegra_dc_writel(dc, value, DC_WIN_SIZE);
> +
> + h_offset = window->src.x * bpp;
> + v_offset = window->src.y;
> + h_size = window->src.w * bpp;
> + v_size = window->src.h;
> +
> + value = V_PRESCALED_SIZE(v_size) | H_PRESCALED_SIZE(h_size);
> + tegra_dc_writel(dc, value, DC_WIN_PRESCALED_SIZE);
> +
> + h_dda = compute_dda_inc(window->src.w, window->dst.w, false, bpp);
> + v_dda = compute_dda_inc(window->src.h, window->dst.h, true, bpp);
> +
> + value = V_DDA_INC(v_dda) | H_DDA_INC(h_dda);
> + tegra_dc_writel(dc, value, DC_WIN_DDA_INC);
> +
> + h_dda = compute_initial_dda(window->src.x);
> + v_dda = compute_initial_dda(window->src.y);
> +
In current implementation, "compute_initial_dda" always returns zero. So
why we need it? Although according to TRM, typically we set H/V initial
dda to zero.
> + tegra_dc_writel(dc, h_dda, DC_WIN_H_INITIAL_DDA);
> + tegra_dc_writel(dc, v_dda, DC_WIN_V_INITIAL_DDA);
> +
> + tegra_dc_writel(dc, 0, DC_WIN_UV_BUF_STRIDE);
> + tegra_dc_writel(dc, 0, DC_WIN_BUF_STRIDE);
> +
> + tegra_dc_writel(dc, window->base, DC_WINBUF_START_ADDR);
> + tegra_dc_writel(dc, window->stride, DC_WIN_LINE_STRIDE);
> + tegra_dc_writel(dc, h_offset, DC_WINBUF_ADDR_H_OFFSET);
> + tegra_dc_writel(dc, v_offset, DC_WINBUF_ADDR_V_OFFSET);
> +
> + value = WIN_ENABLE;
> +
> + if (bpp < 24)
> + value |= COLOR_EXPAND;
> +
> + tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS);
> +
> + /*
> + * Disable blending and assume Window A is the bottom-most window,
> + * Window C is the top-most window and Window B is in the middle.
> + */
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_NOKEY);
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_1WIN);
> +
> + switch (index) {
> + case 0:
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_X);
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_Y);
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_3WIN_XY);
> + break;
> +
> + case 1:
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_X);
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_Y);
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_3WIN_XY);
> + break;
> +
> + case 2:
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_X);
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_Y);
> + tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_3WIN_XY);
> + break;
> + }
> +
> + value = (WIN_A_ACT_REQ << index) |
Re: [PATCH v2 2/5] drm/tegra: Add plane support
On 01/15/2013 12:05 AM, Thierry Reding wrote:
Add support for the B and C planes which support RGB and YUV pixel
formats and can be used as overlays or hardware cursor.
I think hardware cursor has specific meaning for Tegra(e.g: Tegra30
has a 32x32 24bpp or 64x64 2bpp hardware cursor). So you may change it
to hardware accelerated cursor?
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
[...]
+
+static const uint32_t plane_formats[] = {
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_YUV422,
I haven't found something related with YUV format in this patch set. For
example, tegra_dc_format also doesn't take YUV into consideration. So
remove this line.
+};
+
+static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
+{
+ unsigned int i;
+ int err = 0;
+
+ for (i = 0; i 2; i++) {
+ struct tegra_plane *plane;
+
+ plane = devm_kzalloc(drm-dev, sizeof(*plane), GFP_KERNEL);
+ if (!plane)
+ return -ENOMEM;
+
+ plane-index = i;
I suggest to change this line to: plane-index = i + 1;. This makes
the plane's index be consistent with Tegra's windows number. And also we
don't need to worry about passing plane-index + 1 to some functions
which need to know which window is operating on.
+
+ err = drm_plane_init(drm, plane-base, 1 dc-pipe,
+ tegra_plane_funcs, plane_formats,
+ ARRAY_SIZE(plane_formats), false);
+ if (err 0)
+ return err;
+ }
+
+ return 0;
+}
+
[...]
+int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
+ const struct tegra_dc_window *window)
+{
+ unsigned h_offset, v_offset, h_size, v_size, h_dda, v_dda, bpp;
+ unsigned long value;
+
+ bpp = window-bits_per_pixel / 8;
+
+ value = WINDOW_A_SELECT index;
+ tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+ tegra_dc_writel(dc, window-format, DC_WIN_COLOR_DEPTH);
+ tegra_dc_writel(dc, 0, DC_WIN_BYTE_SWAP);
+
+ value = V_POSITION(window-dst.y) | H_POSITION(window-dst.x);
+ tegra_dc_writel(dc, value, DC_WIN_POSITION);
+
+ value = V_SIZE(window-dst.h) | H_SIZE(window-dst.w);
+ tegra_dc_writel(dc, value, DC_WIN_SIZE);
+
+ h_offset = window-src.x * bpp;
+ v_offset = window-src.y;
+ h_size = window-src.w * bpp;
+ v_size = window-src.h;
+
+ value = V_PRESCALED_SIZE(v_size) | H_PRESCALED_SIZE(h_size);
+ tegra_dc_writel(dc, value, DC_WIN_PRESCALED_SIZE);
+
+ h_dda = compute_dda_inc(window-src.w, window-dst.w, false, bpp);
+ v_dda = compute_dda_inc(window-src.h, window-dst.h, true, bpp);
+
+ value = V_DDA_INC(v_dda) | H_DDA_INC(h_dda);
+ tegra_dc_writel(dc, value, DC_WIN_DDA_INC);
+
+ h_dda = compute_initial_dda(window-src.x);
+ v_dda = compute_initial_dda(window-src.y);
+
In current implementation, compute_initial_dda always returns zero. So
why we need it? Although according to TRM, typically we set H/V initial
dda to zero.
+ tegra_dc_writel(dc, h_dda, DC_WIN_H_INITIAL_DDA);
+ tegra_dc_writel(dc, v_dda, DC_WIN_V_INITIAL_DDA);
+
+ tegra_dc_writel(dc, 0, DC_WIN_UV_BUF_STRIDE);
+ tegra_dc_writel(dc, 0, DC_WIN_BUF_STRIDE);
+
+ tegra_dc_writel(dc, window-base, DC_WINBUF_START_ADDR);
+ tegra_dc_writel(dc, window-stride, DC_WIN_LINE_STRIDE);
+ tegra_dc_writel(dc, h_offset, DC_WINBUF_ADDR_H_OFFSET);
+ tegra_dc_writel(dc, v_offset, DC_WINBUF_ADDR_V_OFFSET);
+
+ value = WIN_ENABLE;
+
+ if (bpp 24)
+ value |= COLOR_EXPAND;
+
+ tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS);
+
+ /*
+ * Disable blending and assume Window A is the bottom-most window,
+ * Window C is the top-most window and Window B is in the middle.
+ */
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_NOKEY);
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_1WIN);
+
+ switch (index) {
+ case 0:
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_X);
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_Y);
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_3WIN_XY);
+ break;
+
+ case 1:
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_X);
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_Y);
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_3WIN_XY);
+ break;
+
+ case 2:
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_X);
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_2WIN_Y);
+ tegra_dc_writel(dc, 0x00, DC_WIN_BLEND_3WIN_XY);
+ break;
+ }
+
+ value = (WIN_A_ACT_REQ index) | (WIN_A_UPDATE index);
+ tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+ return 0;
+}
+
+unsigned
[PATCHv4 0/8] Support for Tegra 2D hardware
On 01/03/2013 01:50 PM, Terje Bergstr?m wrote: > On 03.01.2013 05:31, Mark Zhang wrote: >> Sorry I didn't get it. Yes, in current design, you can pin all mem >> handles in one time but I haven't found anything related with "locking >> only once per submit". >> >> My idea is: >> - remove "job->addr_phys" >> - assign "job->reloc_addr_phys" & "job->gather_addr_phys" separately >> - In "pin_job_mem", just call "host1x_memmgr_pin_array_ids" twice to >> fill the "reloc_addr_phy" & "gather_addr_phys". >> >> Anything I misunderstood? > > The current design uses CMA, which makes pinning basically a no-op. When > we have IOMMU support, pinning requires calling into IOMMU. Updating > SMMU tables requires locking, and probably maintenance before SMMU code > also requires its own locked data tables. For example, preventing > duplicate pinning might require a global table of handles. > > Putting all of the handles in one table allows doing duplicate detection > across cmdbuf and reloc tables. This allows pinning each buffer exactly > once, which reduces number of calls to IOMMU. > Thanks Terje. Yes, I understand IOMMU will bring in more operations. But I'm not convinced that separating 2 arrays have lots of overheads than putting them into one array. But it doesn't matter, after the IOMMU support version released, I can read this part of codes again. Let's talk about this at that time. >> "host1x_cma_pin_array_ids" doesn't return negative value right now, so >> maybe you need to take a look at it. > > True, and also a consequence of using CMA: pinning can never fail. With > IOMMU, pinning can fail. Got it. Agree. > > Terje >
[PATCHv4 0/8] Support for Tegra 2D hardware
On 01/02/2013 05:42 PM, Terje Bergstr?m wrote:
> On 28.12.2012 11:14, Mark Zhang wrote:
>> diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
>> index a936902..c3ded60 100644
>> --- a/drivers/gpu/drm/tegra/gr2d.c
>> +++ b/drivers/gpu/drm/tegra/gr2d.c
>> @@ -131,6 +131,14 @@ static int gr2d_submit(struct tegra_drm_context
>> *context,
>> if (err)
>> goto fail;
>>
>> + /* We define CMA as an temporary solution in host1x driver.
>> + That's also why we have a CMA kernel config in it.
>> + But seems here, in tegradrm, we hardcode the CMA here.
>> + So what do you do when host1x change to IOMMU?
>> + Do we also need to define a CMA config in tegradrm
>> driver,
>> + then after host1x changes to IOMMU, we add another IOMMU
>> + config in tegradrm? Or we should invent another more
>> + generic wrapper layer here? */
>> cmdbuf.mem = handle_cma_to_host1x(drm, file_priv,
>> cmdbuf.mem);
>
> Lucas is working on host1x allocator, so let's defer this comment until
> we have Lucas' code.
>
OK.
>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
>> index cc8ca2f..0867b72 100644
>> --- a/drivers/gpu/host1x/job.c
>> +++ b/drivers/gpu/host1x/job.c
>> @@ -82,6 +82,26 @@ struct host1x_job *host1x_job_alloc(struct
>> host1x_channel *ch,
>> mem += num_unpins * sizeof(dma_addr_t);
>> job->pin_ids = num_unpins ? mem : NULL;
>>
>> + /* I think this is a somewhat ugly design.
>> + Actually "addr_phys" is consisted by "reloc_addr_phys" and
>> + "gather_addr_phys".
>> + Why don't we just declare "reloc_addr_phys" and
>> "gather_addr_phys"?
>> + In current design, let's say if one nvhost newbie changes the
>> order
>> + of these 2 blocks of code in function "pin_job_mem":
>> +
>> + for (i = 0; i < job->num_relocs; i++) {
>> + struct host1x_reloc *reloc = >relocarray[i];
>> + job->pin_ids[count] = reloc->target;
>> + count++;
>> + }
>> +
>> + for (i = 0; i < job->num_gathers; i++) {
>> + struct host1x_job_gather *g = >gathers[i];
>> + job->pin_ids[count] = g->mem_id;
>> + count++;
>> + }
>> +
>> + Then likely something weird occurs... */
>
> We do this because this way we can implement batch pinning of memory
> handles. This way we can decrease memory handle management a lot as we
> need to do locking only once per submit.
>
Sorry I didn't get it. Yes, in current design, you can pin all mem
handles in one time but I haven't found anything related with "locking
only once per submit".
My idea is:
- remove "job->addr_phys"
- assign "job->reloc_addr_phys" & "job->gather_addr_phys" separately
- In "pin_job_mem", just call "host1x_memmgr_pin_array_ids" twice to
fill the "reloc_addr_phy" & "gather_addr_phys".
Anything I misunderstood?
> Decreasing memory management overhead is really important, because in
> complex graphics cases, there are might be a hundreds of buffer
> references per submit, and several submits per frame. Any extra overhead
> relates directly to reduced performance.
>
>> job->reloc_addr_phys = job->addr_phys;
>> job->gather_addr_phys = >addr_phys[num_relocs];
>>
>> @@ -252,6 +272,10 @@ static int do_relocs(struct host1x_job *job,
>> }
>> }
>>
>> + /* I have question here. Does this mean the address info
>> + which need to be relocated(according to the libdrm codes,
>> + seems this address is "0xDEADBEEF") always staying at the
>> + beginning of a page? */
>> __raw_writel(
>> (job->reloc_addr_phys[i] +
>> reloc->target_offset) >> reloc->shift,
>
> No - the slot can be anywhere. That's why we have cmdbuf_offset in the
> reloc struct.
>
Yes. Sorry I must misread the code before.
>> @@ -565,6 +589,7 @@ int host1x_job_pin(struct host1x_job *job, struct
>> platform_device *pdev)
>> }
>> }
>>
>
[PATCHv4 0/8] Support for Tegra 2D hardware
On 01/02/2013 05:25 PM, Terje Bergstr?m wrote:
> On 26.12.2012 11:42, Mark Zhang wrote:
[...]
>
>>
>> if (!de)
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
>> index 07e8813..01ed10d 100644
>> --- a/drivers/gpu/host1x/dev.c
>> +++ b/drivers/gpu/host1x/dev.c
>> @@ -38,6 +38,7 @@
>>
>> struct host1x *host1x;
>>
>> +/* Called by drm unlocked ioctl function. So do we need a lock here? */
>> void host1x_syncpt_incr_byid(u32 id)
>
> No, host1x_syncpt_incr_byid is SMP safe.
Correct. Lock is unnecessary.
>
>> {
>> struct host1x_syncpt *sp = host1x->syncpt + id;
>> @@ -52,6 +53,7 @@ u32 host1x_syncpt_read_byid(u32 id)
>> }
>> EXPORT_SYMBOL(host1x_syncpt_read_byid);
>>
>> +/* Called by drm unlocked ioctl function. So do we need a lock here? */
>> int host1x_syncpt_wait_byid(u32 id, u32 thresh, long timeout, u32 *value)
>
> Same here, SMP safe.
>
>> {
>> struct host1x_syncpt *sp = host1x->syncpt + id;
>> @@ -161,6 +163,8 @@ static int host1x_probe(struct platform_device *dev)
>>
>> err = host1x_alloc_resources(host);
>> if (err) {
>> + /* We don't have chip_ops right now, so here the
>> + error message is somewhat improper */
>> dev_err(>dev, "failed to init chip support\n");
>> goto fail;
>> }
>
> Actually, alloc_resources only allocates intr->syncpt, so I the code to
> host1x_intr_init().
>
>> @@ -175,6 +179,14 @@ static int host1x_probe(struct platform_device *dev)
>> if (!host->syncpt)
>> goto fail;
>>
>> + /* I suggest create a dedicate function for initializing nop sp.
>> + First this "_host1x_syncpt_alloc" looks like an internal/static
>> + function.
>> + Then actually "host1x_syncpt_alloc" & "_host1x_syncpt_alloc" all
>> + serve host1x client(e.g: gr2d) so it's not suitable to use them
>> + for nop sp.
>> + Just create a wrapper function to call _host1x_syncpt_alloc is OK.
>> + This will make the code more readable. */
>> host->nop_sp = _host1x_syncpt_alloc(host, NULL, 0);
>
> _host1x_syncpt_alloc is an internal function, not exported.
> host1x_syncpt_alloc is exported. I think it's even better if I just move
> allocation of nop_sp to happen in host1x_syncpt_init.
>
Agree.
>> if (!host->nop_sp)
>> goto fail;
[...]
>
>> diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c
>> index efcb9be..e112001 100644
>> --- a/drivers/gpu/host1x/intr.c
>> +++ b/drivers/gpu/host1x/intr.c
>> @@ -329,8 +329,13 @@ void host1x_intr_deinit(struct host1x_intr *intr)
>> void host1x_intr_start(struct host1x_intr *intr, u32 hz)
>> {
>> struct host1x *host1x = intr_to_host1x(intr);
>> +
>> + /* Why we need to lock here? Seems like this function is
>> + called by host1x's probe only. */
>> mutex_lock(>mutex);
>>
>> + /* "init_host_sync" has already been called in function
>> + host1x_intr_init. Remove this line. */
>> host1x->intr_op.init_host_sync(intr);
>> host1x->intr_op.set_host_clocks_per_usec(intr,
>> DIV_ROUND_UP(hz, 100));
>
> In future, we'll call host1x_intr_start() whenever host1x is turned on.
> Thus we need locking.
>
> init_host_sync() should actually be called from host1x_intr_start(), not
> _init().
>
Got it. Thanks for explanation.
>> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
>> index 07cbca5..9a234ad 100644
>> --- a/drivers/gpu/host1x/syncpt.c
>> +++ b/drivers/gpu/host1x/syncpt.c
>> @@ -309,6 +309,8 @@ struct host1x_syncpt *host1x_syncpt_init(struct
>> host1x *host)
>> struct host1x_syncpt *syncpt, *sp;
>> int i;
>>
>> + /* Consider devm_kzalloc here. Then you can forget the release
>> + stuffs about this "syncpt". */
>> syncpt = sp = kzalloc(sizeof(struct host1x_syncpt) *
>> host->info.nb_pts,
>> GFP_KERNEL);
>> if (!syncpt)
>
> Will do.
>
> Thanks!
>
> Terje
>
[PATCHv4 3/8] gpu: host1x: Add channel support
On 01/02/2013 05:31 PM, Terje Bergstr?m wrote:
> On 02.01.2013 09:40, Mark Zhang wrote:
>> On 12/21/2012 07:39 PM, Terje Bergstrom wrote:
>>> Add support for host1x client modules, and host1x channels to submit
>>> work to the clients. The work is submitted in GEM CMA buffers, so
>>> this patch adds support for them.
>>>
>>> Signed-off-by: Terje Bergstrom
>>> ---
>> [...]
>>> +/*
>>> + * Begin a cdma submit
>>> + */
>>> +int host1x_cdma_begin(struct host1x_cdma *cdma, struct host1x_job *job)
>>> +{
>>> + struct host1x *host1x = cdma_to_host1x(cdma);
>>> +
>>> + mutex_lock(>lock);
>>> +
>>> + if (job->timeout) {
>>> + /* init state on first submit with timeout value */
>>> + if (!cdma->timeout.initialized) {
>>> + int err;
>>> + err = host1x->cdma_op.timeout_init(cdma,
>>> + job->syncpt_id);
>>> + if (err) {
>>> + mutex_unlock(>lock);
>>> + return err;
>>> + }
>>> + }
>>> + }
>>> + if (!cdma->running)
>>> + host1x->cdma_op.start(cdma);
>>> +
>>> + cdma->slots_free = 0;
>>> + cdma->slots_used = 0;
>>> + cdma->first_get = host1x->cdma_pb_op.putptr(>push_buffer);
>>> +
>>> + trace_host1x_cdma_begin(job->ch->dev->name);
>>
>> Seems missing "mutex_unlock(>lock);" here.
>
> That's intentional. Writing a job to channel is atomic, so lock is taken
> from host1x_cdma_begin() until host1x_cdma_end().
>
Okay. So can we consider that lock and unlock this mutex in the function
which calls host1x_cdma_begin and host1x_cdma_end, that means, in
current implementation, function "channel_submit"?
Mark
> Terje
>
[PATCHv4 3/8] gpu: host1x: Add channel support
Just one minor issue. Check below.
On 12/21/2012 07:39 PM, Terje Bergstrom wrote:
> Add support for host1x client modules, and host1x channels to submit
> work to the clients. The work is submitted in GEM CMA buffers, so
> this patch adds support for them.
>
> Signed-off-by: Terje Bergstrom
> ---
[...]
> +/*
> + * Begin a cdma submit
> + */
> +int host1x_cdma_begin(struct host1x_cdma *cdma, struct host1x_job *job)
> +{
> + struct host1x *host1x = cdma_to_host1x(cdma);
> +
> + mutex_lock(>lock);
> +
> + if (job->timeout) {
> + /* init state on first submit with timeout value */
> + if (!cdma->timeout.initialized) {
> + int err;
> + err = host1x->cdma_op.timeout_init(cdma,
> + job->syncpt_id);
> + if (err) {
> + mutex_unlock(>lock);
> + return err;
> + }
> + }
> + }
> + if (!cdma->running)
> + host1x->cdma_op.start(cdma);
> +
> + cdma->slots_free = 0;
> + cdma->slots_used = 0;
> + cdma->first_get = host1x->cdma_pb_op.putptr(>push_buffer);
> +
> + trace_host1x_cdma_begin(job->ch->dev->name);
Seems missing "mutex_unlock(>lock);" here.
> + return 0;
> +}
[...]
> +
> #endif /* _TRACE_HOST1X_H */
>
> /* This part must be outside protection */
>
[RFC,libdrm 1/3] tegra: Add stream library
On 01/02/2013 02:31 PM, Terje Bergstr?m wrote: > On 02.01.2013 04:44, Mark Zhang wrote: >> Agree. If we are able to do something dynamically, normally that'll be >> better. >> >> Terje, we can get the Tegra version in FUSE. I think we don't need this >> kind of try-catch logics. > > We'd need to have in user space a map of SoC version -> number of sync > points. As kernel already has this and checks for invalid fences, I > don't think user space needs the same check. > > We're anyway talking about a possibility of coding error, nothing that > can happen on properly working software, so I wouldn't want to add any > code that isn't absolutely necessary to catch that. > Ah, yes, my bad. It's libdrm, not host1x driver. Mark > Terje >
[RFC,libdrm 1/3] tegra: Add stream library
On 12/31/2012 02:22 PM, Terje Bergstr?m wrote: > On 28.12.2012 22:48, Thierry Reding wrote: >> I disagree. We shouldn't be hiding this kind of detail behind an #ifdef. >> Instead it should be detected at runtime. Otherwise you'll need to build >> different versions of libdrm for every generation of Tegra. That may be >> fine for NVIDIA provided BSPs, but distributions would have a very hard >> time dealing with that. What we want is software that works unmodified >> on as many generations of Tegra as possible. > > I agree. The fences will be rejected by kernel and kernel knows about > number of sync points in each host1x revision. So, we could just submit > and look at return code. > Agree. If we are able to do something dynamically, normally that'll be better. Terje, we can get the Tegra version in FUSE. I think we don't need this kind of try-catch logics. Mark > Terje >
Re: [PATCHv4 3/8] gpu: host1x: Add channel support
On 01/02/2013 05:31 PM, Terje Bergström wrote:
On 02.01.2013 09:40, Mark Zhang wrote:
On 12/21/2012 07:39 PM, Terje Bergstrom wrote:
Add support for host1x client modules, and host1x channels to submit
work to the clients. The work is submitted in GEM CMA buffers, so
this patch adds support for them.
Signed-off-by: Terje Bergstrom tbergst...@nvidia.com
---
[...]
+/*
+ * Begin a cdma submit
+ */
+int host1x_cdma_begin(struct host1x_cdma *cdma, struct host1x_job *job)
+{
+ struct host1x *host1x = cdma_to_host1x(cdma);
+
+ mutex_lock(cdma-lock);
+
+ if (job-timeout) {
+ /* init state on first submit with timeout value */
+ if (!cdma-timeout.initialized) {
+ int err;
+ err = host1x-cdma_op.timeout_init(cdma,
+ job-syncpt_id);
+ if (err) {
+ mutex_unlock(cdma-lock);
+ return err;
+ }
+ }
+ }
+ if (!cdma-running)
+ host1x-cdma_op.start(cdma);
+
+ cdma-slots_free = 0;
+ cdma-slots_used = 0;
+ cdma-first_get = host1x-cdma_pb_op.putptr(cdma-push_buffer);
+
+ trace_host1x_cdma_begin(job-ch-dev-name);
Seems missing mutex_unlock(cdma-lock); here.
That's intentional. Writing a job to channel is atomic, so lock is taken
from host1x_cdma_begin() until host1x_cdma_end().
Okay. So can we consider that lock and unlock this mutex in the function
which calls host1x_cdma_begin and host1x_cdma_end, that means, in
current implementation, function channel_submit?
Mark
Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv4 0/8] Support for Tegra 2D hardware
On 01/02/2013 05:25 PM, Terje Bergström wrote:
On 26.12.2012 11:42, Mark Zhang wrote:
[...]
if (!de)
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 07e8813..01ed10d 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -38,6 +38,7 @@
struct host1x *host1x;
+/* Called by drm unlocked ioctl function. So do we need a lock here? */
void host1x_syncpt_incr_byid(u32 id)
No, host1x_syncpt_incr_byid is SMP safe.
Correct. Lock is unnecessary.
{
struct host1x_syncpt *sp = host1x-syncpt + id;
@@ -52,6 +53,7 @@ u32 host1x_syncpt_read_byid(u32 id)
}
EXPORT_SYMBOL(host1x_syncpt_read_byid);
+/* Called by drm unlocked ioctl function. So do we need a lock here? */
int host1x_syncpt_wait_byid(u32 id, u32 thresh, long timeout, u32 *value)
Same here, SMP safe.
{
struct host1x_syncpt *sp = host1x-syncpt + id;
@@ -161,6 +163,8 @@ static int host1x_probe(struct platform_device *dev)
err = host1x_alloc_resources(host);
if (err) {
+ /* We don't have chip_ops right now, so here the
+ error message is somewhat improper */
dev_err(dev-dev, failed to init chip support\n);
goto fail;
}
Actually, alloc_resources only allocates intr-syncpt, so I the code to
host1x_intr_init().
@@ -175,6 +179,14 @@ static int host1x_probe(struct platform_device *dev)
if (!host-syncpt)
goto fail;
+ /* I suggest create a dedicate function for initializing nop sp.
+ First this _host1x_syncpt_alloc looks like an internal/static
+ function.
+ Then actually host1x_syncpt_alloc _host1x_syncpt_alloc all
+ serve host1x client(e.g: gr2d) so it's not suitable to use them
+ for nop sp.
+ Just create a wrapper function to call _host1x_syncpt_alloc is OK.
+ This will make the code more readable. */
host-nop_sp = _host1x_syncpt_alloc(host, NULL, 0);
_host1x_syncpt_alloc is an internal function, not exported.
host1x_syncpt_alloc is exported. I think it's even better if I just move
allocation of nop_sp to happen in host1x_syncpt_init.
Agree.
if (!host-nop_sp)
goto fail;
[...]
diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c
index efcb9be..e112001 100644
--- a/drivers/gpu/host1x/intr.c
+++ b/drivers/gpu/host1x/intr.c
@@ -329,8 +329,13 @@ void host1x_intr_deinit(struct host1x_intr *intr)
void host1x_intr_start(struct host1x_intr *intr, u32 hz)
{
struct host1x *host1x = intr_to_host1x(intr);
+
+ /* Why we need to lock here? Seems like this function is
+ called by host1x's probe only. */
mutex_lock(intr-mutex);
+ /* init_host_sync has already been called in function
+ host1x_intr_init. Remove this line. */
host1x-intr_op.init_host_sync(intr);
host1x-intr_op.set_host_clocks_per_usec(intr,
DIV_ROUND_UP(hz, 100));
In future, we'll call host1x_intr_start() whenever host1x is turned on.
Thus we need locking.
init_host_sync() should actually be called from host1x_intr_start(), not
_init().
Got it. Thanks for explanation.
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index 07cbca5..9a234ad 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -309,6 +309,8 @@ struct host1x_syncpt *host1x_syncpt_init(struct
host1x *host)
struct host1x_syncpt *syncpt, *sp;
int i;
+ /* Consider devm_kzalloc here. Then you can forget the release
+ stuffs about this syncpt. */
syncpt = sp = kzalloc(sizeof(struct host1x_syncpt) *
host-info.nb_pts,
GFP_KERNEL);
if (!syncpt)
Will do.
Thanks!
Terje
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv4 0/8] Support for Tegra 2D hardware
On 01/02/2013 05:42 PM, Terje Bergström wrote:
On 28.12.2012 11:14, Mark Zhang wrote:
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index a936902..c3ded60 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -131,6 +131,14 @@ static int gr2d_submit(struct tegra_drm_context
*context,
if (err)
goto fail;
+ /* We define CMA as an temporary solution in host1x driver.
+ That's also why we have a CMA kernel config in it.
+ But seems here, in tegradrm, we hardcode the CMA here.
+ So what do you do when host1x change to IOMMU?
+ Do we also need to define a CMA config in tegradrm
driver,
+ then after host1x changes to IOMMU, we add another IOMMU
+ config in tegradrm? Or we should invent another more
+ generic wrapper layer here? */
cmdbuf.mem = handle_cma_to_host1x(drm, file_priv,
cmdbuf.mem);
Lucas is working on host1x allocator, so let's defer this comment until
we have Lucas' code.
OK.
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index cc8ca2f..0867b72 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -82,6 +82,26 @@ struct host1x_job *host1x_job_alloc(struct
host1x_channel *ch,
mem += num_unpins * sizeof(dma_addr_t);
job-pin_ids = num_unpins ? mem : NULL;
+ /* I think this is a somewhat ugly design.
+ Actually addr_phys is consisted by reloc_addr_phys and
+ gather_addr_phys.
+ Why don't we just declare reloc_addr_phys and
gather_addr_phys?
+ In current design, let's say if one nvhost newbie changes the
order
+ of these 2 blocks of code in function pin_job_mem:
+
+ for (i = 0; i job-num_relocs; i++) {
+ struct host1x_reloc *reloc = job-relocarray[i];
+ job-pin_ids[count] = reloc-target;
+ count++;
+ }
+
+ for (i = 0; i job-num_gathers; i++) {
+ struct host1x_job_gather *g = job-gathers[i];
+ job-pin_ids[count] = g-mem_id;
+ count++;
+ }
+
+ Then likely something weird occurs... */
We do this because this way we can implement batch pinning of memory
handles. This way we can decrease memory handle management a lot as we
need to do locking only once per submit.
Sorry I didn't get it. Yes, in current design, you can pin all mem
handles in one time but I haven't found anything related with locking
only once per submit.
My idea is:
- remove job-addr_phys
- assign job-reloc_addr_phys job-gather_addr_phys separately
- In pin_job_mem, just call host1x_memmgr_pin_array_ids twice to
fill the reloc_addr_phy gather_addr_phys.
Anything I misunderstood?
Decreasing memory management overhead is really important, because in
complex graphics cases, there are might be a hundreds of buffer
references per submit, and several submits per frame. Any extra overhead
relates directly to reduced performance.
job-reloc_addr_phys = job-addr_phys;
job-gather_addr_phys = job-addr_phys[num_relocs];
@@ -252,6 +272,10 @@ static int do_relocs(struct host1x_job *job,
}
}
+ /* I have question here. Does this mean the address info
+ which need to be relocated(according to the libdrm codes,
+ seems this address is 0xDEADBEEF) always staying at the
+ beginning of a page? */
__raw_writel(
(job-reloc_addr_phys[i] +
reloc-target_offset) reloc-shift,
No - the slot can be anywhere. That's why we have cmdbuf_offset in the
reloc struct.
Yes. Sorry I must misread the code before.
@@ -565,6 +589,7 @@ int host1x_job_pin(struct host1x_job *job, struct
platform_device *pdev)
}
}
+ /* if (host1x_firewall !err) { */
if (host1x_firewall) {
err = copy_gathers(job, pdev);
if (err) {
Will add.
@@ -573,6 +598,9 @@ int host1x_job_pin(struct host1x_job *job, struct
platform_device *pdev)
}
}
+/* Rename this label to out or something else.
+ Because if everything goes right, the codes under this label also
+ get executed. */
fail:
wmb();
Will do.
diff --git a/drivers/gpu/host1x/memmgr.c b/drivers/gpu/host1x/memmgr.c
index f3954f7..bb5763d 100644
--- a/drivers/gpu/host1x/memmgr.c
+++ b/drivers/gpu/host1x/memmgr.c
@@ -156,6 +156,9 @@ int host1x_memmgr_pin_array_ids(struct
platform_device *dev,
count, unpin_data[pin_count],
phys_addr);
+ /* I don't think the current host1x_cma_pin_array_ids
Re: [PATCHv4 0/8] Support for Tegra 2D hardware
On 01/03/2013 01:50 PM, Terje Bergström wrote: On 03.01.2013 05:31, Mark Zhang wrote: Sorry I didn't get it. Yes, in current design, you can pin all mem handles in one time but I haven't found anything related with locking only once per submit. My idea is: - remove job-addr_phys - assign job-reloc_addr_phys job-gather_addr_phys separately - In pin_job_mem, just call host1x_memmgr_pin_array_ids twice to fill the reloc_addr_phy gather_addr_phys. Anything I misunderstood? The current design uses CMA, which makes pinning basically a no-op. When we have IOMMU support, pinning requires calling into IOMMU. Updating SMMU tables requires locking, and probably maintenance before SMMU code also requires its own locked data tables. For example, preventing duplicate pinning might require a global table of handles. Putting all of the handles in one table allows doing duplicate detection across cmdbuf and reloc tables. This allows pinning each buffer exactly once, which reduces number of calls to IOMMU. Thanks Terje. Yes, I understand IOMMU will bring in more operations. But I'm not convinced that separating 2 arrays have lots of overheads than putting them into one array. But it doesn't matter, after the IOMMU support version released, I can read this part of codes again. Let's talk about this at that time. host1x_cma_pin_array_ids doesn't return negative value right now, so maybe you need to take a look at it. True, and also a consequence of using CMA: pinning can never fail. With IOMMU, pinning can fail. Got it. Agree. Terje ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv4 3/8] gpu: host1x: Add channel support
Just one minor issue. Check below.
On 12/21/2012 07:39 PM, Terje Bergstrom wrote:
Add support for host1x client modules, and host1x channels to submit
work to the clients. The work is submitted in GEM CMA buffers, so
this patch adds support for them.
Signed-off-by: Terje Bergstrom tbergst...@nvidia.com
---
[...]
+/*
+ * Begin a cdma submit
+ */
+int host1x_cdma_begin(struct host1x_cdma *cdma, struct host1x_job *job)
+{
+ struct host1x *host1x = cdma_to_host1x(cdma);
+
+ mutex_lock(cdma-lock);
+
+ if (job-timeout) {
+ /* init state on first submit with timeout value */
+ if (!cdma-timeout.initialized) {
+ int err;
+ err = host1x-cdma_op.timeout_init(cdma,
+ job-syncpt_id);
+ if (err) {
+ mutex_unlock(cdma-lock);
+ return err;
+ }
+ }
+ }
+ if (!cdma-running)
+ host1x-cdma_op.start(cdma);
+
+ cdma-slots_free = 0;
+ cdma-slots_used = 0;
+ cdma-first_get = host1x-cdma_pb_op.putptr(cdma-push_buffer);
+
+ trace_host1x_cdma_begin(job-ch-dev-name);
Seems missing mutex_unlock(cdma-lock); here.
+ return 0;
+}
[...]
+
#endif /* _TRACE_HOST1X_H */
/* This part must be outside protection */
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCHv4 0/8] Support for Tegra 2D hardware
Hi Terje,
Here is the second part comments. I admit I still haven't finished
reading the codes... really too many codes. :)
Anyway I'll keep doing this when I have free slots.
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index a936902..c3ded60 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -131,6 +131,14 @@ static int gr2d_submit(struct tegra_drm_context
*context,
if (err)
goto fail;
+ /* We define CMA as an temporary solution in host1x driver.
+ That's also why we have a CMA kernel config in it.
+ But seems here, in tegradrm, we hardcode the CMA here.
+ So what do you do when host1x change to IOMMU?
+ Do we also need to define a CMA config in tegradrm
driver,
+ then after host1x changes to IOMMU, we add another IOMMU
+ config in tegradrm? Or we should invent another more
+ generic wrapper layer here? */
cmdbuf.mem = handle_cma_to_host1x(drm, file_priv,
cmdbuf.mem);
if (!cmdbuf.mem)
goto fail;
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index cc8ca2f..0867b72 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -82,6 +82,26 @@ struct host1x_job *host1x_job_alloc(struct
host1x_channel *ch,
mem += num_unpins * sizeof(dma_addr_t);
job->pin_ids = num_unpins ? mem : NULL;
+ /* I think this is a somewhat ugly design.
+ Actually "addr_phys" is consisted by "reloc_addr_phys" and
+ "gather_addr_phys".
+ Why don't we just declare "reloc_addr_phys" and
"gather_addr_phys"?
+ In current design, let's say if one nvhost newbie changes the
order
+ of these 2 blocks of code in function "pin_job_mem":
+
+ for (i = 0; i < job->num_relocs; i++) {
+ struct host1x_reloc *reloc = >relocarray[i];
+ job->pin_ids[count] = reloc->target;
+ count++;
+ }
+
+ for (i = 0; i < job->num_gathers; i++) {
+ struct host1x_job_gather *g = >gathers[i];
+ job->pin_ids[count] = g->mem_id;
+ count++;
+ }
+
+ Then likely something weird occurs... */
job->reloc_addr_phys = job->addr_phys;
job->gather_addr_phys = >addr_phys[num_relocs];
@@ -252,6 +272,10 @@ static int do_relocs(struct host1x_job *job,
}
}
+ /* I have question here. Does this mean the address info
+ which need to be relocated(according to the libdrm codes,
+ seems this address is "0xDEADBEEF") always staying at the
+ beginning of a page? */
__raw_writel(
(job->reloc_addr_phys[i] +
reloc->target_offset) >> reloc->shift,
@@ -565,6 +589,7 @@ int host1x_job_pin(struct host1x_job *job, struct
platform_device *pdev)
}
}
+ /* if (host1x_firewall && !err) { */
if (host1x_firewall) {
err = copy_gathers(job, pdev);
if (err) {
@@ -573,6 +598,9 @@ int host1x_job_pin(struct host1x_job *job, struct
platform_device *pdev)
}
}
+/* Rename this label to "out" or something else.
+ Because if everything goes right, the codes under this label also
+ get executed. */
fail:
wmb();
diff --git a/drivers/gpu/host1x/memmgr.c b/drivers/gpu/host1x/memmgr.c
index f3954f7..bb5763d 100644
--- a/drivers/gpu/host1x/memmgr.c
+++ b/drivers/gpu/host1x/memmgr.c
@@ -156,6 +156,9 @@ int host1x_memmgr_pin_array_ids(struct
platform_device *dev,
count, _data[pin_count],
phys_addr);
+ /* I don't think the current "host1x_cma_pin_array_ids"
+ is able to return a negative value. So this "if" doesn't
+ make sense...*/
if (cma_count < 0) {
/* clean up previous handles */
while (pin_count) {
Mark
On 12/21/2012 07:39 PM, Terje Bergstrom wrote:
> This set of patches adds support for Tegra20 and Tegra30 host1x and
> 2D. It is based on linux-next-20121220.
>
> The fourth version has only few changes compared to previous version:
> * Fixed some sparse warnings
> * Fixed host1x Makefile to allow building as module
> * Fixed host1x module unload
> * Fixed tegradrm unload sequence
> * host1x creates DRM dummy device and tegradrm uses it for storing
>DRM related data.
>
> Some of the issues left open:
> * Register definitions still use static inline. There has been a
>debate about code style versus ability to use compiler type
>checking and code coverage analysis. There was no conclusion, so
>I left it as was.
> * Uses still CMA helpers.
[RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 04:50 PM, Arto Merilainen wrote:
> On 12/28/2012 09:57 AM, Mark Zhang wrote:
>> On 12/28/2012 03:45 PM, Arto Merilainen wrote:
>>> On 12/28/2012 08:47 AM, Mark Zhang wrote:
>>>>> +
>>>>> +/* Add fences */
>>>>> +if (num_fences) {
>>>>> +
>>>>> +tegra_stream_push(stream,
>>>>> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
>>>>> +host1x_uclass_wait_syncpt_r(), num_fences));
>>>>> +
>>>>> +for (; num_fences; num_fences--, fence++) {
>>>>> +assert(tegra_fence_is_valid(fence));
>>>>
>>>> This is useless. We already add "1 + num_fences" to num_words above. So
>>>> move this "assert" before adding "1 + num_fences" to num_words makes
>>>> sense.
>>>>
>>>
>>> The assertion checks the validity of a single fence - not if there is
>>> room in the command buffer.
>>>
>>> The goal is to prevent having invalid fences in the command stream. If
>>> this check were not here it would be possible to initialise a fence with
>>> tegra_fence_clear() and put that fence into the stream.
>>
>> My idea is, if one fence is invalid, then we should not count this in
>> "num_words". In current code, if one fence is invalid, then this fence
>> will not be pushed into the command stream, and the "num_words" shows a
>> wrong command buffer size.
>>
>> So I think we should:
>> - validate the fences, remove the invalid fence
>> - update num_words
>> - then you don't need to check fence here - I mean, before push a host1x
>> syncpt wait command into the active buffer of stream.
>>
>
> In my opinion asking tegra_stream_begin() to put a bad fence into the
> stream is a case we should never be. assert() kills the application
> immediately (in debug builds) and usually this helps the programmer for
> 1) finding bugs 2) not doing bad code.
>
Yep, I agree. But in release builds, assert does nothing. So this
checking doesn't make sense and also a wrong fence will be pushed into
command buffer silently. And we always use release version in real
products, so we can't count on this "assert".
> "Silencing" is not a good solution especially in this case:
> tegra_stream_flush() returns an invalid fence when flushing fails. If
> the application chains submits (i.e. do a blit and then do another using
> the output of the first blit) it is crucial to be sure the first submit
> has been performed before starting the second one.
>
Yes. So I suggest doing fence checking at the beginning of the
"tegra_stream_begin", if invalid fence found, return an error.
> - Arto
[RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 03:45 PM, Arto Merilainen wrote:
> On 12/28/2012 08:47 AM, Mark Zhang wrote:
>>> +int tegra_fence_is_valid(const struct tegra_fence *fence)
>>> +{
>>> +int valid = fence ? 1 : 0;
>>> +valid = valid && fence->id != (uint32_t) -1;
>>> +valid = valid && fence->id < 32;
>>
>> Hardcode here. Assume always has 32 syncpts.
>> Change to a micro wrapped with tegra version ifdef will be better.
>>
>
> You are correct. I will fix this.
>
>>> +return valid;
>>> +}
>>> +
>> [...]
>>> +
>>> +/* Add fences */
>>> +if (num_fences) {
>>> +
>>> +tegra_stream_push(stream,
>>> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
>>> +host1x_uclass_wait_syncpt_r(), num_fences));
>>> +
>>> +for (; num_fences; num_fences--, fence++) {
>>> +assert(tegra_fence_is_valid(fence));
>>
>> This is useless. We already add "1 + num_fences" to num_words above. So
>> move this "assert" before adding "1 + num_fences" to num_words makes
>> sense.
>>
>
> The assertion checks the validity of a single fence - not if there is
> room in the command buffer.
>
> The goal is to prevent having invalid fences in the command stream. If
> this check were not here it would be possible to initialise a fence with
> tegra_fence_clear() and put that fence into the stream.
My idea is, if one fence is invalid, then we should not count this in
"num_words". In current code, if one fence is invalid, then this fence
will not be pushed into the command stream, and the "num_words" shows a
wrong command buffer size.
So I think we should:
- validate the fences, remove the invalid fence
- update num_words
- then you don't need to check fence here - I mean, before push a host1x
syncpt wait command into the active buffer of stream.
>
>>> +
>>> +tegra_stream_push(stream,
>>> nvhost_class_host_wait_syncpt(fence->id,
>>> +fence->value));
>>> +}
>>> +}
>>> +
>>> +if (class_id)
>>> +tegra_stream_push(stream, nvhost_opcode_setclass(class_id,
>>> 0, 0));
>>> +
>>> +return 0;
>>> +}
>>> +
>> [...]
>>> +
>>> +#endif /* TEGRA_DRMIF_H */
>>>
>
> - Arto
[RFC,libdrm 2/3] tegra: Add 2d library
I just skimmed the code of libdrm while I'm trying to understand the
host1x driver. So below is what I found.
Mark
On 12/13/2012 10:01 PM, Arto Meril?inen wrote:
> From: Francis Hart
>
> This patch introduces a simple 2d library on top of stream library.
>
> Signed-off-by: Francis Hart
[...]
> +void
> +g2fill_dispatch(const struct tegra_2d_g2fill *hw,
> +struct tegra_stream *stream)
> +{
> +ASSERT(hw);
> +ASSERT(hw->is_valid);
> +ASSERT(stream);
> +
> +tegra_stream_push_setclass(stream, NV_GRAPHICS_2D_CLASS_ID);
I think at the end of the "tegra_stream_begin", we already pushed a
setclass opcode into the stream's active buffer.
So why we need another one here?
> +
> +tegra_stream_push_words(stream,
> +>block,
> +sizeof(hw->block) / sizeof(uint32_t),
> +1,
> +tegra_reloc(>block.dstba,
> +hw->dst_handle,
> +hw->dst_offset));
> +}
> +
[...]
> +#endif // TEGRA_2D_H
>
[RFC,libdrm 1/3] tegra: Add stream library
I just skimmed the code of libdrm while I'm trying to understand the
host1x driver. So below is what I found.
Mark
On 12/13/2012 10:01 PM, Arto Meril?inen wrote:
> From: Arto Merilainen
>
> This patch introduces tegra stream library. The library is used for
> buffer management, command stream construction and work
> synchronization.
>
[...]
> +
> +/*
> + * tegra_fence_is_valid(fence)
> + *
> + * Check validity of a fence. We just check that the fence range
> + * is valid w.r.t. host1x hardware.
> + */
> +
> +int tegra_fence_is_valid(const struct tegra_fence *fence)
> +{
> +int valid = fence ? 1 : 0;
> +valid = valid && fence->id != (uint32_t) -1;
> +valid = valid && fence->id < 32;
Hardcode here. Assume always has 32 syncpts.
Change to a micro wrapped with tegra version ifdef will be better.
> +return valid;
> +}
> +
[...]
> +
> +/* Add fences */
> +if (num_fences) {
> +
> +tegra_stream_push(stream,
> +nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
> +host1x_uclass_wait_syncpt_r(), num_fences));
> +
> +for (; num_fences; num_fences--, fence++) {
> +assert(tegra_fence_is_valid(fence));
This is useless. We already add "1 + num_fences" to num_words above. So
move this "assert" before adding "1 + num_fences" to num_words makes sense.
> +
> +tegra_stream_push(stream,
> nvhost_class_host_wait_syncpt(fence->id,
> +fence->value));
> +}
> +}
> +
> +if (class_id)
> +tegra_stream_push(stream, nvhost_opcode_setclass(class_id, 0, 0));
> +
> +return 0;
> +}
> +
[...]
> +
> +#endif /* TEGRA_DRMIF_H */
>
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 04:50 PM, Arto Merilainen wrote:
On 12/28/2012 09:57 AM, Mark Zhang wrote:
On 12/28/2012 03:45 PM, Arto Merilainen wrote:
On 12/28/2012 08:47 AM, Mark Zhang wrote:
+
+/* Add fences */
+if (num_fences) {
+
+tegra_stream_push(stream,
+nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
+host1x_uclass_wait_syncpt_r(), num_fences));
+
+for (; num_fences; num_fences--, fence++) {
+assert(tegra_fence_is_valid(fence));
This is useless. We already add 1 + num_fences to num_words above. So
move this assert before adding 1 + num_fences to num_words makes
sense.
The assertion checks the validity of a single fence - not if there is
room in the command buffer.
The goal is to prevent having invalid fences in the command stream. If
this check were not here it would be possible to initialise a fence with
tegra_fence_clear() and put that fence into the stream.
My idea is, if one fence is invalid, then we should not count this in
num_words. In current code, if one fence is invalid, then this fence
will not be pushed into the command stream, and the num_words shows a
wrong command buffer size.
So I think we should:
- validate the fences, remove the invalid fence
- update num_words
- then you don't need to check fence here - I mean, before push a host1x
syncpt wait command into the active buffer of stream.
In my opinion asking tegra_stream_begin() to put a bad fence into the
stream is a case we should never be. assert() kills the application
immediately (in debug builds) and usually this helps the programmer for
1) finding bugs 2) not doing bad code.
Yep, I agree. But in release builds, assert does nothing. So this
checking doesn't make sense and also a wrong fence will be pushed into
command buffer silently. And we always use release version in real
products, so we can't count on this assert.
Silencing is not a good solution especially in this case:
tegra_stream_flush() returns an invalid fence when flushing fails. If
the application chains submits (i.e. do a blit and then do another using
the output of the first blit) it is crucial to be sure the first submit
has been performed before starting the second one.
Yes. So I suggest doing fence checking at the beginning of the
tegra_stream_begin, if invalid fence found, return an error.
- Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv4 0/8] Support for Tegra 2D hardware
Hi Terje,
Here is the second part comments. I admit I still haven't finished
reading the codes... really too many codes. :)
Anyway I'll keep doing this when I have free slots.
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index a936902..c3ded60 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -131,6 +131,14 @@ static int gr2d_submit(struct tegra_drm_context
*context,
if (err)
goto fail;
+ /* We define CMA as an temporary solution in host1x driver.
+ That's also why we have a CMA kernel config in it.
+ But seems here, in tegradrm, we hardcode the CMA here.
+ So what do you do when host1x change to IOMMU?
+ Do we also need to define a CMA config in tegradrm
driver,
+ then after host1x changes to IOMMU, we add another IOMMU
+ config in tegradrm? Or we should invent another more
+ generic wrapper layer here? */
cmdbuf.mem = handle_cma_to_host1x(drm, file_priv,
cmdbuf.mem);
if (!cmdbuf.mem)
goto fail;
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index cc8ca2f..0867b72 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -82,6 +82,26 @@ struct host1x_job *host1x_job_alloc(struct
host1x_channel *ch,
mem += num_unpins * sizeof(dma_addr_t);
job-pin_ids = num_unpins ? mem : NULL;
+ /* I think this is a somewhat ugly design.
+ Actually addr_phys is consisted by reloc_addr_phys and
+ gather_addr_phys.
+ Why don't we just declare reloc_addr_phys and
gather_addr_phys?
+ In current design, let's say if one nvhost newbie changes the
order
+ of these 2 blocks of code in function pin_job_mem:
+
+ for (i = 0; i job-num_relocs; i++) {
+ struct host1x_reloc *reloc = job-relocarray[i];
+ job-pin_ids[count] = reloc-target;
+ count++;
+ }
+
+ for (i = 0; i job-num_gathers; i++) {
+ struct host1x_job_gather *g = job-gathers[i];
+ job-pin_ids[count] = g-mem_id;
+ count++;
+ }
+
+ Then likely something weird occurs... */
job-reloc_addr_phys = job-addr_phys;
job-gather_addr_phys = job-addr_phys[num_relocs];
@@ -252,6 +272,10 @@ static int do_relocs(struct host1x_job *job,
}
}
+ /* I have question here. Does this mean the address info
+ which need to be relocated(according to the libdrm codes,
+ seems this address is 0xDEADBEEF) always staying at the
+ beginning of a page? */
__raw_writel(
(job-reloc_addr_phys[i] +
reloc-target_offset) reloc-shift,
@@ -565,6 +589,7 @@ int host1x_job_pin(struct host1x_job *job, struct
platform_device *pdev)
}
}
+ /* if (host1x_firewall !err) { */
if (host1x_firewall) {
err = copy_gathers(job, pdev);
if (err) {
@@ -573,6 +598,9 @@ int host1x_job_pin(struct host1x_job *job, struct
platform_device *pdev)
}
}
+/* Rename this label to out or something else.
+ Because if everything goes right, the codes under this label also
+ get executed. */
fail:
wmb();
diff --git a/drivers/gpu/host1x/memmgr.c b/drivers/gpu/host1x/memmgr.c
index f3954f7..bb5763d 100644
--- a/drivers/gpu/host1x/memmgr.c
+++ b/drivers/gpu/host1x/memmgr.c
@@ -156,6 +156,9 @@ int host1x_memmgr_pin_array_ids(struct
platform_device *dev,
count, unpin_data[pin_count],
phys_addr);
+ /* I don't think the current host1x_cma_pin_array_ids
+ is able to return a negative value. So this if doesn't
+ make sense...*/
if (cma_count 0) {
/* clean up previous handles */
while (pin_count) {
Mark
On 12/21/2012 07:39 PM, Terje Bergstrom wrote:
This set of patches adds support for Tegra20 and Tegra30 host1x and
2D. It is based on linux-next-20121220.
The fourth version has only few changes compared to previous version:
* Fixed some sparse warnings
* Fixed host1x Makefile to allow building as module
* Fixed host1x module unload
* Fixed tegradrm unload sequence
* host1x creates DRM dummy device and tegradrm uses it for storing
DRM related data.
Some of the issues left open:
* Register definitions still use static inline. There has been a
debate about code style versus ability to use compiler type
checking and code coverage analysis. There was no conclusion, so
I left it as was.
* Uses still CMA helpers. IOMMU support will replace CMA with host1x
Re: [RFC,libdrm 1/3] tegra: Add stream library
I just skimmed the code of libdrm while I'm trying to understand the
host1x driver. So below is what I found.
Mark
On 12/13/2012 10:01 PM, Arto Meriläinen wrote:
From: Arto Merilainen amerilai...@nvidia.com
This patch introduces tegra stream library. The library is used for
buffer management, command stream construction and work
synchronization.
[...]
+
+/*
+ * tegra_fence_is_valid(fence)
+ *
+ * Check validity of a fence. We just check that the fence range
+ * is valid w.r.t. host1x hardware.
+ */
+
+int tegra_fence_is_valid(const struct tegra_fence *fence)
+{
+int valid = fence ? 1 : 0;
+valid = valid fence-id != (uint32_t) -1;
+valid = valid fence-id 32;
Hardcode here. Assume always has 32 syncpts.
Change to a micro wrapped with tegra version ifdef will be better.
+return valid;
+}
+
[...]
+
+/* Add fences */
+if (num_fences) {
+
+tegra_stream_push(stream,
+nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
+host1x_uclass_wait_syncpt_r(), num_fences));
+
+for (; num_fences; num_fences--, fence++) {
+assert(tegra_fence_is_valid(fence));
This is useless. We already add 1 + num_fences to num_words above. So
move this assert before adding 1 + num_fences to num_words makes sense.
+
+tegra_stream_push(stream,
nvhost_class_host_wait_syncpt(fence-id,
+fence-value));
+}
+}
+
+if (class_id)
+tegra_stream_push(stream, nvhost_opcode_setclass(class_id, 0, 0));
+
+return 0;
+}
+
[...]
+
+#endif /* TEGRA_DRMIF_H */
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,libdrm 1/3] tegra: Add stream library
On 12/28/2012 03:45 PM, Arto Merilainen wrote:
On 12/28/2012 08:47 AM, Mark Zhang wrote:
+int tegra_fence_is_valid(const struct tegra_fence *fence)
+{
+int valid = fence ? 1 : 0;
+valid = valid fence-id != (uint32_t) -1;
+valid = valid fence-id 32;
Hardcode here. Assume always has 32 syncpts.
Change to a micro wrapped with tegra version ifdef will be better.
You are correct. I will fix this.
+return valid;
+}
+
[...]
+
+/* Add fences */
+if (num_fences) {
+
+tegra_stream_push(stream,
+nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
+host1x_uclass_wait_syncpt_r(), num_fences));
+
+for (; num_fences; num_fences--, fence++) {
+assert(tegra_fence_is_valid(fence));
This is useless. We already add 1 + num_fences to num_words above. So
move this assert before adding 1 + num_fences to num_words makes
sense.
The assertion checks the validity of a single fence - not if there is
room in the command buffer.
The goal is to prevent having invalid fences in the command stream. If
this check were not here it would be possible to initialise a fence with
tegra_fence_clear() and put that fence into the stream.
My idea is, if one fence is invalid, then we should not count this in
num_words. In current code, if one fence is invalid, then this fence
will not be pushed into the command stream, and the num_words shows a
wrong command buffer size.
So I think we should:
- validate the fences, remove the invalid fence
- update num_words
- then you don't need to check fence here - I mean, before push a host1x
syncpt wait command into the active buffer of stream.
+
+tegra_stream_push(stream,
nvhost_class_host_wait_syncpt(fence-id,
+fence-value));
+}
+}
+
+if (class_id)
+tegra_stream_push(stream, nvhost_opcode_setclass(class_id,
0, 0));
+
+return 0;
+}
+
[...]
+
+#endif /* TEGRA_DRMIF_H */
- Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCHv4 0/8] Support for Tegra 2D hardware
Hi Terje,
I applied your patches on top of upstream 1224 kernel. Then I read the
codes. So here is my review comments(I use "git diff" to print out,
check below). I admit it's easy for me to not need to find the
corresponding lines in your 8 patch mails, but I've no idea whether it
is ok for you. If this makes you not feeling good, I'll do this in old
ways. Basically, I think in this diff output, there are filename/line
number/function name, so it should not be a hard work for you to
understand my comments.
P.S: I haven't finished the review so here is what I found today.
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index a936902..28954b3 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -247,6 +247,10 @@ static int __devinit gr2d_probe(struct
platform_device *dev)
{
int err;
struct gr2d *gr2d = NULL;
+ /* Cause drm_device is created in host1x driver. So
+ if host1x drivers loads after tegradrm, then in this
+ gr2d_probe function, this "drm_device" will be NULL.
+ How can we handle this? Defer driver probe? */
struct platform_device *drm_device =
host1x_drm_device(to_platform_device(dev->dev.parent));
struct tegradrm *tegradrm = platform_get_drvdata(drm_device);
@@ -273,6 +277,11 @@ static int __devinit gr2d_probe(struct
platform_device *dev)
gr2d->syncpt = host1x_syncpt_alloc(dev, 0);
if (!gr2d->syncpt) {
+ /* Do we really need this?
+ After "host1x_channel_alloc", the refcount of this
+ channel is 0. So calling host1x_channel_put here will
+ make the refcount going to negative.
+ I suppose we should call "host1x_free_channel" here. */
host1x_channel_put(gr2d->channel);
return -ENOMEM;
}
@@ -284,6 +293,7 @@ static int __devinit gr2d_probe(struct
platform_device *dev)
err = tegra_drm_register_client(tegradrm, >client);
if (err)
+ /* Add "host1x_free_channel" */
return err;
platform_set_drvdata(dev, gr2d);
diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c
index 3705cae..ed83b9e 100644
--- a/drivers/gpu/host1x/channel.c
+++ b/drivers/gpu/host1x/channel.c
@@ -95,6 +95,9 @@ struct host1x_channel *host1x_channel_alloc(struct
platform_device *pdev)
int max_channels = host1x->info.nb_channels;
int err;
+ /* Is it necessary to add mutex protection here?
+ I'm just wondering in a smp system, multiple host1x clients
+ may try to alloc their channels simultaneously... */
if (chindex > max_channels)
return NULL;
diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
index 86d5c70..f2bd5aa 100644
--- a/drivers/gpu/host1x/debug.c
+++ b/drivers/gpu/host1x/debug.c
@@ -164,6 +164,10 @@ static const struct file_operations
host1x_debug_fops = {
void host1x_debug_init(struct host1x *host1x)
{
+ /* I think it's better to set this directory name the same with
+ the driver's name -- defined in dev.c:
+ #define DRIVER_NAME "tegra-host1x"
+ */
struct dentry *de = debugfs_create_dir("tegra_host", NULL);
if (!de)
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 07e8813..01ed10d 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -38,6 +38,7 @@
struct host1x *host1x;
+/* Called by drm unlocked ioctl function. So do we need a lock here? */
void host1x_syncpt_incr_byid(u32 id)
{
struct host1x_syncpt *sp = host1x->syncpt + id;
@@ -52,6 +53,7 @@ u32 host1x_syncpt_read_byid(u32 id)
}
EXPORT_SYMBOL(host1x_syncpt_read_byid);
+/* Called by drm unlocked ioctl function. So do we need a lock here? */
int host1x_syncpt_wait_byid(u32 id, u32 thresh, long timeout, u32 *value)
{
struct host1x_syncpt *sp = host1x->syncpt + id;
@@ -161,6 +163,8 @@ static int host1x_probe(struct platform_device *dev)
err = host1x_alloc_resources(host);
if (err) {
+ /* We don't have chip_ops right now, so here the
+ error message is somewhat improper */
dev_err(>dev, "failed to init chip support\n");
goto fail;
}
@@ -175,6 +179,14 @@ static int host1x_probe(struct platform_device *dev)
if (!host->syncpt)
goto fail;
+ /* I suggest create a dedicate function for initializing nop sp.
+ First this "_host1x_syncpt_alloc" looks like an internal/static
+ function.
+ Then actually "host1x_syncpt_alloc" & "_host1x_syncpt_alloc" all
+ serve host1x client(e.g: gr2d) so it's not suitable to use them
+ for nop sp.
+ Just create a wrapper function to call _host1x_syncpt_alloc is OK.
+ This will make the code more readable. */
host->nop_sp =
Re: [PATCHv4 0/8] Support for Tegra 2D hardware
Hi Terje,
I applied your patches on top of upstream 1224 kernel. Then I read the
codes. So here is my review comments(I use git diff to print out,
check below). I admit it's easy for me to not need to find the
corresponding lines in your 8 patch mails, but I've no idea whether it
is ok for you. If this makes you not feeling good, I'll do this in old
ways. Basically, I think in this diff output, there are filename/line
number/function name, so it should not be a hard work for you to
understand my comments.
P.S: I haven't finished the review so here is what I found today.
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index a936902..28954b3 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -247,6 +247,10 @@ static int __devinit gr2d_probe(struct
platform_device *dev)
{
int err;
struct gr2d *gr2d = NULL;
+ /* Cause drm_device is created in host1x driver. So
+ if host1x drivers loads after tegradrm, then in this
+ gr2d_probe function, this drm_device will be NULL.
+ How can we handle this? Defer driver probe? */
struct platform_device *drm_device =
host1x_drm_device(to_platform_device(dev-dev.parent));
struct tegradrm *tegradrm = platform_get_drvdata(drm_device);
@@ -273,6 +277,11 @@ static int __devinit gr2d_probe(struct
platform_device *dev)
gr2d-syncpt = host1x_syncpt_alloc(dev, 0);
if (!gr2d-syncpt) {
+ /* Do we really need this?
+ After host1x_channel_alloc, the refcount of this
+ channel is 0. So calling host1x_channel_put here will
+ make the refcount going to negative.
+ I suppose we should call host1x_free_channel here. */
host1x_channel_put(gr2d-channel);
return -ENOMEM;
}
@@ -284,6 +293,7 @@ static int __devinit gr2d_probe(struct
platform_device *dev)
err = tegra_drm_register_client(tegradrm, gr2d-client);
if (err)
+ /* Add host1x_free_channel */
return err;
platform_set_drvdata(dev, gr2d);
diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c
index 3705cae..ed83b9e 100644
--- a/drivers/gpu/host1x/channel.c
+++ b/drivers/gpu/host1x/channel.c
@@ -95,6 +95,9 @@ struct host1x_channel *host1x_channel_alloc(struct
platform_device *pdev)
int max_channels = host1x-info.nb_channels;
int err;
+ /* Is it necessary to add mutex protection here?
+ I'm just wondering in a smp system, multiple host1x clients
+ may try to alloc their channels simultaneously... */
if (chindex max_channels)
return NULL;
diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
index 86d5c70..f2bd5aa 100644
--- a/drivers/gpu/host1x/debug.c
+++ b/drivers/gpu/host1x/debug.c
@@ -164,6 +164,10 @@ static const struct file_operations
host1x_debug_fops = {
void host1x_debug_init(struct host1x *host1x)
{
+ /* I think it's better to set this directory name the same with
+ the driver's name -- defined in dev.c:
+ #define DRIVER_NAME tegra-host1x
+ */
struct dentry *de = debugfs_create_dir(tegra_host, NULL);
if (!de)
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 07e8813..01ed10d 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -38,6 +38,7 @@
struct host1x *host1x;
+/* Called by drm unlocked ioctl function. So do we need a lock here? */
void host1x_syncpt_incr_byid(u32 id)
{
struct host1x_syncpt *sp = host1x-syncpt + id;
@@ -52,6 +53,7 @@ u32 host1x_syncpt_read_byid(u32 id)
}
EXPORT_SYMBOL(host1x_syncpt_read_byid);
+/* Called by drm unlocked ioctl function. So do we need a lock here? */
int host1x_syncpt_wait_byid(u32 id, u32 thresh, long timeout, u32 *value)
{
struct host1x_syncpt *sp = host1x-syncpt + id;
@@ -161,6 +163,8 @@ static int host1x_probe(struct platform_device *dev)
err = host1x_alloc_resources(host);
if (err) {
+ /* We don't have chip_ops right now, so here the
+ error message is somewhat improper */
dev_err(dev-dev, failed to init chip support\n);
goto fail;
}
@@ -175,6 +179,14 @@ static int host1x_probe(struct platform_device *dev)
if (!host-syncpt)
goto fail;
+ /* I suggest create a dedicate function for initializing nop sp.
+ First this _host1x_syncpt_alloc looks like an internal/static
+ function.
+ Then actually host1x_syncpt_alloc _host1x_syncpt_alloc all
+ serve host1x client(e.g: gr2d) so it's not suitable to use them
+ for nop sp.
+ Just create a wrapper function to call _host1x_syncpt_alloc is OK.
+ This will make the code more readable. */
host-nop_sp = _host1x_syncpt_alloc(host,
[PATCH 5/6] drm: tegra: clean out old gem prototypes
We must forget about that. I believe no one read the header files while review. So thanks. :) Mark On 12/20/2012 05:38 AM, Lucas Stach wrote: > There is no gem.c anymore, those functions are implemented by the > drm_cma_helpers now. > > Signed-off-by: Lucas Stach > --- > drivers/gpu/drm/tegra/drm.h | 18 -- > 1 Datei ge?ndert, 18 Zeilen entfernt(-) > > diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h > index eae1f56..71e61f2 100644 > --- a/drivers/gpu/drm/tegra/drm.h > +++ b/drivers/gpu/drm/tegra/drm.h > @@ -205,24 +205,6 @@ extern int tegra_output_parse_dt(struct tegra_output > *output); > extern int tegra_output_init(struct drm_device *drm, struct tegra_output > *output); > extern int tegra_output_exit(struct tegra_output *output); > > -/* from gem.c */ > -extern struct tegra_gem_object *tegra_gem_alloc(struct drm_device *drm, > - size_t size); > -extern int tegra_gem_handle_create(struct drm_device *drm, > -struct drm_file *file, size_t size, > -unsigned long flags, uint32_t *handle); > -extern int tegra_gem_dumb_create(struct drm_file *file, struct drm_device > *drm, > - struct drm_mode_create_dumb *args); > -extern int tegra_gem_dumb_map_offset(struct drm_file *file, > - struct drm_device *drm, uint32_t handle, > - uint64_t *offset); > -extern int tegra_gem_dumb_destroy(struct drm_file *file, > - struct drm_device *drm, uint32_t handle); > -extern int tegra_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); > -extern int tegra_gem_init_object(struct drm_gem_object *obj); > -extern void tegra_gem_free_object(struct drm_gem_object *obj); > -extern struct vm_operations_struct tegra_gem_vm_ops; > - > /* from fb.c */ > extern int tegra_drm_fb_init(struct drm_device *drm); > extern void tegra_drm_fb_exit(struct drm_device *drm); >
