diff mbox

[v3,3/7] drm/tegra: Implement .mode_set_base()

Message ID 1360771506-17849-4-git-send-email-thierry.reding@avionic-design.de (mailing list archive)
State New, archived
Headers show

Commit Message

Thierry Reding Feb. 13, 2013, 4:05 p.m. UTC
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.reding@avionic-design.de>
---
Changes in v3:
- split DC_CMD_STATE_CONTROL writes

 drivers/gpu/drm/tegra/dc.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Mark Zhang Feb. 18, 2013, 6:17 a.m. UTC | #1
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.reding@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,
>
Mark Zhang Feb. 18, 2013, 6:28 a.m. UTC | #2
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.reding@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,
>>
Thierry Reding Feb. 18, 2013, 6:48 a.m. UTC | #3
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.reding@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.

Thierry
Mark Zhang Feb. 18, 2013, 7:06 a.m. UTC | #4
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.reding@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
>
Thierry Reding Feb. 18, 2013, 7:20 a.m. UTC | #5
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.reding@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.

Thierry
Mark Zhang Feb. 18, 2013, 8:40 a.m. UTC | #6
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.reding@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
>
Mark Zhang Feb. 18, 2013, 9:07 a.m. UTC | #7
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
>>
diff mbox

Patch

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;
+}
+
 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,