| Message ID | 1415226371-1880-4-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> On 11/06/2014 12:26 AM, Jesse Barnes wrote: > If these change (e.g. after a modeset following a fastboot), we need to > do a full mode set. > > v2: > - put under pipe_config check so we don't deref a null state (Jesse) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cb96f11..c86eee6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > &modeset_pipes, > &prepare_pipes, > &disable_pipes); > - if (IS_ERR(pipe_config)) > + if (IS_ERR(pipe_config)) { > goto fail; > + } else if (pipe_config) { > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > + to_intel_crtc(set->crtc)->config.has_audio) > + config->mode_changed = true; > + > + /* Force mode sets for any infoframe stuff */ > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > + to_intel_crtc(set->crtc)->config.has_infoframe) > + config->mode_changed = true; > + } > > /* set_mode will free it in the mode_changed case */ > if (!config->mode_changed) >
On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > If these change (e.g. after a modeset following a fastboot), we need to > do a full mode set. > > v2: > - put under pipe_config check so we don't deref a null state (Jesse) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cb96f11..c86eee6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > &modeset_pipes, > &prepare_pipes, > &disable_pipes); > - if (IS_ERR(pipe_config)) > + if (IS_ERR(pipe_config)) { > goto fail; > + } else if (pipe_config) { > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > + to_intel_crtc(set->crtc)->config.has_audio) > + config->mode_changed = true; > + > + /* Force mode sets for any infoframe stuff */ > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > + to_intel_crtc(set->crtc)->config.has_infoframe) Jesse, is "||" correct here? This is bound to force a lot of mode sets. See https://bugs.freedesktop.org/show_bug.cgi?id=86683 BR, Jani. > + config->mode_changed = true; > + } > > /* set_mode will free it in the mode_changed case */ > if (!config->mode_changed) > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 01 Dec 2014 12:25:45 +0200 Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > If these change (e.g. after a modeset following a fastboot), we need to > > do a full mode set. > > > > v2: > > - put under pipe_config check so we don't deref a null state (Jesse) > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index cb96f11..c86eee6 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > &modeset_pipes, > > &prepare_pipes, > > &disable_pipes); > > - if (IS_ERR(pipe_config)) > > + if (IS_ERR(pipe_config)) { > > goto fail; > > + } else if (pipe_config) { > > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > > + to_intel_crtc(set->crtc)->config.has_audio) > > + config->mode_changed = true; > > + > > + /* Force mode sets for any infoframe stuff */ > > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > > + to_intel_crtc(set->crtc)->config.has_infoframe) > > Jesse, is "||" correct here? This is bound to force a lot of mode sets. > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683 Yeah, we talked about it on IRC. I was being extra conservative here, since we don't yet have code to analyze whether we need a full mode set due to something that will require an infoframe change. Currently, the only way for us to write a new infoframe is through a mode set, and the old code would just happen to do a mode set most of the time we wanted it to (i.e. when the mode timings changed), but I don't think we'd ever thought of the implications of infoframe changes for stuff that doesn't require a mode set. We could drop this hunk and continue to play it fast & loose, but what we really need here is a fuller check about whether we need a mode set due to requiring an infoframe change, which means pulling them apart a bit.
On Mon, Dec 01, 2014 at 08:04:30AM -0800, Jesse Barnes wrote: > On Mon, 01 Dec 2014 12:25:45 +0200 > Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > If these change (e.g. after a modeset following a fastboot), we need to > > > do a full mode set. > > > > > > v2: > > > - put under pipe_config check so we don't deref a null state (Jesse) > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index cb96f11..c86eee6 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > > &modeset_pipes, > > > &prepare_pipes, > > > &disable_pipes); > > > - if (IS_ERR(pipe_config)) > > > + if (IS_ERR(pipe_config)) { > > > goto fail; > > > + } else if (pipe_config) { > > > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > > > + to_intel_crtc(set->crtc)->config.has_audio) > > > + config->mode_changed = true; > > > + > > > + /* Force mode sets for any infoframe stuff */ > > > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > > > + to_intel_crtc(set->crtc)->config.has_infoframe) > > > > Jesse, is "||" correct here? This is bound to force a lot of mode sets. > > > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683 > > Yeah, we talked about it on IRC. I was being extra conservative here, > since we don't yet have code to analyze whether we need a full mode set > due to something that will require an infoframe change. Currently, the > only way for us to write a new infoframe is through a mode set, and the > old code would just happen to do a mode set most of the time we wanted > it to (i.e. when the mode timings changed), but I don't think we'd ever > thought of the implications of infoframe changes for stuff that doesn't > require a mode set. > > We could drop this hunk and continue to play it fast & loose, but what > we really need here is a fuller check about whether we need a mode set > due to requiring an infoframe change, which means pulling them apart a > bit. We need to be conservative. If we spot extraneous modesets in the wild, we will be motivated to fix it. As a compromise, if you include a DRM_DEBUG_KMS("infoframes detected - forcing full modeset\n") that should be sufficient clue for us to pick up the thread again later. -Chris
On Mon, Dec 01, 2014 at 12:25:45PM +0200, Jani Nikula wrote: > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > If these change (e.g. after a modeset following a fastboot), we need to > > do a full mode set. > > > > v2: > > - put under pipe_config check so we don't deref a null state (Jesse) > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index cb96f11..c86eee6 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > &modeset_pipes, > > &prepare_pipes, > > &disable_pipes); > > - if (IS_ERR(pipe_config)) > > + if (IS_ERR(pipe_config)) { > > goto fail; > > + } else if (pipe_config) { > > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > > + to_intel_crtc(set->crtc)->config.has_audio) > > + config->mode_changed = true; > > + > > + /* Force mode sets for any infoframe stuff */ > > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > > + to_intel_crtc(set->crtc)->config.has_infoframe) > > Jesse, is "||" correct here? This is bound to force a lot of mode sets. > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683 Indeed. And I think the approach is the wrong way round. Imo we should check whether the computed pipe_config matches and have a few specific cases that we can handle when it mismatches (e.g. pipe_src_w/h). Plus some special logic if quirk flags are set (so that we force a full modeset in cases like this to get rid of the boot-up takever state quirk flag). I think best course of action here is to revert this patch, also because it's kinda at the 1w deadline with set for regression. Even though this here is imo QA's fault for taking way too long to deliver the bisect result and noticing the issue (the patch is almost 1 month old by now). Jesse? Thanks, Daniel
On Mon, Dec 01, 2014 at 04:09:07PM +0000, Chris Wilson wrote: > On Mon, Dec 01, 2014 at 08:04:30AM -0800, Jesse Barnes wrote: > > On Mon, 01 Dec 2014 12:25:45 +0200 > > Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > > > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > > If these change (e.g. after a modeset following a fastboot), we need to > > > > do a full mode set. > > > > > > > > v2: > > > > - put under pipe_config check so we don't deref a null state (Jesse) > > > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index cb96f11..c86eee6 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > > > &modeset_pipes, > > > > &prepare_pipes, > > > > &disable_pipes); > > > > - if (IS_ERR(pipe_config)) > > > > + if (IS_ERR(pipe_config)) { > > > > goto fail; > > > > + } else if (pipe_config) { > > > > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > > > > + to_intel_crtc(set->crtc)->config.has_audio) > > > > + config->mode_changed = true; > > > > + > > > > + /* Force mode sets for any infoframe stuff */ > > > > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > > > > + to_intel_crtc(set->crtc)->config.has_infoframe) > > > > > > Jesse, is "||" correct here? This is bound to force a lot of mode sets. > > > > > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683 > > > > Yeah, we talked about it on IRC. I was being extra conservative here, > > since we don't yet have code to analyze whether we need a full mode set > > due to something that will require an infoframe change. Currently, the > > only way for us to write a new infoframe is through a mode set, and the > > old code would just happen to do a mode set most of the time we wanted > > it to (i.e. when the mode timings changed), but I don't think we'd ever > > thought of the implications of infoframe changes for stuff that doesn't > > require a mode set. > > > > We could drop this hunk and continue to play it fast & loose, but what > > we really need here is a fuller check about whether we need a mode set > > due to requiring an infoframe change, which means pulling them apart a > > bit. > > We need to be conservative. If we spot extraneous modesets in the wild, > we will be motivated to fix it. As a compromise, if you include a > > DRM_DEBUG_KMS("infoframes detected - forcing full modeset\n") > > that should be sufficient clue for us to pick up the thread again later. The problem is that with the current code we'll do a modeset for all hdmi outputs, unconditionally. Even without fastboot=1 set. Which is too much and a regression. E.g. just having virtual > screen and scrolling with the mouse old-style would now result in a modeset each time we move one pixel. Or at least that's my analysis here (didnt test). -Daniel
On Mon, Dec 01, 2014 at 05:30:41PM +0100, Daniel Vetter wrote: > On Mon, Dec 01, 2014 at 04:09:07PM +0000, Chris Wilson wrote: > > On Mon, Dec 01, 2014 at 08:04:30AM -0800, Jesse Barnes wrote: > > > On Mon, 01 Dec 2014 12:25:45 +0200 > > > Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > > > > > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > > > If these change (e.g. after a modeset following a fastboot), we need to > > > > > do a full mode set. > > > > > > > > > > v2: > > > > > - put under pipe_config check so we don't deref a null state (Jesse) > > > > > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > index cb96f11..c86eee6 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > > > > &modeset_pipes, > > > > > &prepare_pipes, > > > > > &disable_pipes); > > > > > - if (IS_ERR(pipe_config)) > > > > > + if (IS_ERR(pipe_config)) { > > > > > goto fail; > > > > > + } else if (pipe_config) { > > > > > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > > > > > + to_intel_crtc(set->crtc)->config.has_audio) > > > > > + config->mode_changed = true; > > > > > + > > > > > + /* Force mode sets for any infoframe stuff */ > > > > > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > > > > > + to_intel_crtc(set->crtc)->config.has_infoframe) > > > > > > > > Jesse, is "||" correct here? This is bound to force a lot of mode sets. > > > > > > > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683 > > > > > > Yeah, we talked about it on IRC. I was being extra conservative here, > > > since we don't yet have code to analyze whether we need a full mode set > > > due to something that will require an infoframe change. Currently, the > > > only way for us to write a new infoframe is through a mode set, and the > > > old code would just happen to do a mode set most of the time we wanted > > > it to (i.e. when the mode timings changed), but I don't think we'd ever > > > thought of the implications of infoframe changes for stuff that doesn't > > > require a mode set. > > > > > > We could drop this hunk and continue to play it fast & loose, but what > > > we really need here is a fuller check about whether we need a mode set > > > due to requiring an infoframe change, which means pulling them apart a > > > bit. > > > > We need to be conservative. If we spot extraneous modesets in the wild, > > we will be motivated to fix it. As a compromise, if you include a > > > > DRM_DEBUG_KMS("infoframes detected - forcing full modeset\n") > > > > that should be sufficient clue for us to pick up the thread again later. > > The problem is that with the current code we'll do a modeset for all hdmi > outputs, unconditionally. Even without fastboot=1 set. Which is too much > and a regression. E.g. just having virtual > screen and scrolling with the > mouse old-style would now result in a modeset each time we move one pixel. > Or at least that's my analysis here (didnt test). Would it be just as easy to construct a scenario that had an infroframe change that didn't get applied with the revert? Besides which a full modeset on every pan should be good motiviation to make them nonblocking (or at least use vblank workers to do the clenaup) ;-) -Chris
On Mon, 1 Dec 2014 17:16:25 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Dec 01, 2014 at 12:25:45PM +0200, Jani Nikula wrote: > > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > If these change (e.g. after a modeset following a fastboot), we need to > > > do a full mode set. > > > > > > v2: > > > - put under pipe_config check so we don't deref a null state (Jesse) > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index cb96f11..c86eee6 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > > &modeset_pipes, > > > &prepare_pipes, > > > &disable_pipes); > > > - if (IS_ERR(pipe_config)) > > > + if (IS_ERR(pipe_config)) { > > > goto fail; > > > + } else if (pipe_config) { > > > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > > > + to_intel_crtc(set->crtc)->config.has_audio) > > > + config->mode_changed = true; > > > + > > > + /* Force mode sets for any infoframe stuff */ > > > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > > > + to_intel_crtc(set->crtc)->config.has_infoframe) > > > > Jesse, is "||" correct here? This is bound to force a lot of mode sets. > > > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683 > > Indeed. And I think the approach is the wrong way round. Imo we should > check whether the computed pipe_config matches and have a few specific > cases that we can handle when it mismatches (e.g. pipe_src_w/h). Plus some > special logic if quirk flags are set (so that we force a full modeset in > cases like this to get rid of the boot-up takever state quirk flag). > > I think best course of action here is to revert this patch, also because > it's kinda at the 1w deadline with set for regression. Even though this > here is imo QA's fault for taking way too long to deliver the bisect > result and noticing the issue (the patch is almost 1 month old by now). > > Jesse? I'd rather just drop the infoframe check if that's causing trouble. The audio one should probably still stay.
On Mon, Dec 01, 2014 at 04:37:41PM +0000, Chris Wilson wrote: > On Mon, Dec 01, 2014 at 05:30:41PM +0100, Daniel Vetter wrote: > > On Mon, Dec 01, 2014 at 04:09:07PM +0000, Chris Wilson wrote: > > > On Mon, Dec 01, 2014 at 08:04:30AM -0800, Jesse Barnes wrote: > > > > On Mon, 01 Dec 2014 12:25:45 +0200 > > > > Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > > > > > > > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > > > > If these change (e.g. after a modeset following a fastboot), we need to > > > > > > do a full mode set. > > > > > > > > > > > > v2: > > > > > > - put under pipe_config check so we don't deref a null state (Jesse) > > > > > > > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > > > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > > index cb96f11..c86eee6 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > > > > > &modeset_pipes, > > > > > > &prepare_pipes, > > > > > > &disable_pipes); > > > > > > - if (IS_ERR(pipe_config)) > > > > > > + if (IS_ERR(pipe_config)) { > > > > > > goto fail; > > > > > > + } else if (pipe_config) { > > > > > > + if (to_intel_crtc(set->crtc)->new_config->has_audio != > > > > > > + to_intel_crtc(set->crtc)->config.has_audio) > > > > > > + config->mode_changed = true; > > > > > > + > > > > > > + /* Force mode sets for any infoframe stuff */ > > > > > > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || > > > > > > + to_intel_crtc(set->crtc)->config.has_infoframe) > > > > > > > > > > Jesse, is "||" correct here? This is bound to force a lot of mode sets. > > > > > > > > > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683 > > > > > > > > Yeah, we talked about it on IRC. I was being extra conservative here, > > > > since we don't yet have code to analyze whether we need a full mode set > > > > due to something that will require an infoframe change. Currently, the > > > > only way for us to write a new infoframe is through a mode set, and the > > > > old code would just happen to do a mode set most of the time we wanted > > > > it to (i.e. when the mode timings changed), but I don't think we'd ever > > > > thought of the implications of infoframe changes for stuff that doesn't > > > > require a mode set. > > > > > > > > We could drop this hunk and continue to play it fast & loose, but what > > > > we really need here is a fuller check about whether we need a mode set > > > > due to requiring an infoframe change, which means pulling them apart a > > > > bit. > > > > > > We need to be conservative. If we spot extraneous modesets in the wild, > > > we will be motivated to fix it. As a compromise, if you include a > > > > > > DRM_DEBUG_KMS("infoframes detected - forcing full modeset\n") > > > > > > that should be sufficient clue for us to pick up the thread again later. > > > > The problem is that with the current code we'll do a modeset for all hdmi > > outputs, unconditionally. Even without fastboot=1 set. Which is too much > > and a regression. E.g. just having virtual > screen and scrolling with the > > mouse old-style would now result in a modeset each time we move one pixel. > > Or at least that's my analysis here (didnt test). > > Would it be just as easy to construct a scenario that had an infroframe > change that didn't get applied with the revert? Besides which a full > modeset on every pan should be good motiviation to make them nonblocking > (or at least use vblank workers to do the clenaup) ;-) Without fastboot we still force a full modeset at boot-up (that part hasn't been cleanup up yet and brought out from under the fastboot switch). So for users that don't set special options reverting this won't cause any more modesets. -Daniel
On Mon, 01 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Dec 01, 2014 at 04:37:41PM +0000, Chris Wilson wrote: >> On Mon, Dec 01, 2014 at 05:30:41PM +0100, Daniel Vetter wrote: >> > On Mon, Dec 01, 2014 at 04:09:07PM +0000, Chris Wilson wrote: >> > > On Mon, Dec 01, 2014 at 08:04:30AM -0800, Jesse Barnes wrote: >> > > > On Mon, 01 Dec 2014 12:25:45 +0200 >> > > > Jani Nikula <jani.nikula@linux.intel.com> wrote: >> > > > >> > > > > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > > > > > If these change (e.g. after a modeset following a fastboot), we need to >> > > > > > do a full mode set. >> > > > > > >> > > > > > v2: >> > > > > > - put under pipe_config check so we don't deref a null state (Jesse) >> > > > > > >> > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> > > > > > --- >> > > > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- >> > > > > > 1 file changed, 11 insertions(+), 1 deletion(-) >> > > > > > >> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > > > > > index cb96f11..c86eee6 100644 >> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c >> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c >> > > > > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) >> > > > > > &modeset_pipes, >> > > > > > &prepare_pipes, >> > > > > > &disable_pipes); >> > > > > > - if (IS_ERR(pipe_config)) >> > > > > > + if (IS_ERR(pipe_config)) { >> > > > > > goto fail; >> > > > > > + } else if (pipe_config) { >> > > > > > + if (to_intel_crtc(set->crtc)->new_config->has_audio != >> > > > > > + to_intel_crtc(set->crtc)->config.has_audio) >> > > > > > + config->mode_changed = true; >> > > > > > + >> > > > > > + /* Force mode sets for any infoframe stuff */ >> > > > > > + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || >> > > > > > + to_intel_crtc(set->crtc)->config.has_infoframe) >> > > > > >> > > > > Jesse, is "||" correct here? This is bound to force a lot of mode sets. >> > > > > >> > > > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683 >> > > > >> > > > Yeah, we talked about it on IRC. I was being extra conservative here, >> > > > since we don't yet have code to analyze whether we need a full mode set >> > > > due to something that will require an infoframe change. Currently, the >> > > > only way for us to write a new infoframe is through a mode set, and the >> > > > old code would just happen to do a mode set most of the time we wanted >> > > > it to (i.e. when the mode timings changed), but I don't think we'd ever >> > > > thought of the implications of infoframe changes for stuff that doesn't >> > > > require a mode set. >> > > > >> > > > We could drop this hunk and continue to play it fast & loose, but what >> > > > we really need here is a fuller check about whether we need a mode set >> > > > due to requiring an infoframe change, which means pulling them apart a >> > > > bit. >> > > >> > > We need to be conservative. If we spot extraneous modesets in the wild, >> > > we will be motivated to fix it. As a compromise, if you include a >> > > >> > > DRM_DEBUG_KMS("infoframes detected - forcing full modeset\n") >> > > >> > > that should be sufficient clue for us to pick up the thread again later. >> > >> > The problem is that with the current code we'll do a modeset for all hdmi >> > outputs, unconditionally. Even without fastboot=1 set. Which is too much >> > and a regression. E.g. just having virtual > screen and scrolling with the >> > mouse old-style would now result in a modeset each time we move one pixel. >> > Or at least that's my analysis here (didnt test). >> >> Would it be just as easy to construct a scenario that had an infroframe >> change that didn't get applied with the revert? Besides which a full >> modeset on every pan should be good motiviation to make them nonblocking >> (or at least use vblank workers to do the clenaup) ;-) > > Without fastboot we still force a full modeset at boot-up (that part > hasn't been cleanup up yet and brought out from under the fastboot > switch). So for users that don't set special options reverting this won't > cause any more modesets. The bug doesn't mention any special options. BR, Jani. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Dec 1, 2014 at 6:35 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote: >>> Would it be just as easy to construct a scenario that had an infroframe >>> change that didn't get applied with the revert? Besides which a full >>> modeset on every pan should be good motiviation to make them nonblocking >>> (or at least use vblank workers to do the clenaup) ;-) >> >> Without fastboot we still force a full modeset at boot-up (that part >> hasn't been cleanup up yet and brought out from under the fastboot >> switch). So for users that don't set special options reverting this won't >> cause any more modesets. > > The bug doesn't mention any special options. This was a reply to Chris' question whether there's a scenario where reverting would cause problems, and my answer was "without fastboot=1 no". Unrelated to the bug, but related to the revert. The bug of course happens irrespective of fastboot. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cb96f11..c86eee6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set) &modeset_pipes, &prepare_pipes, &disable_pipes); - if (IS_ERR(pipe_config)) + if (IS_ERR(pipe_config)) { goto fail; + } else if (pipe_config) { + if (to_intel_crtc(set->crtc)->new_config->has_audio != + to_intel_crtc(set->crtc)->config.has_audio) + config->mode_changed = true; + + /* Force mode sets for any infoframe stuff */ + if (to_intel_crtc(set->crtc)->new_config->has_infoframe || + to_intel_crtc(set->crtc)->config.has_infoframe) + config->mode_changed = true; + } /* set_mode will free it in the mode_changed case */ if (!config->mode_changed)
If these change (e.g. after a modeset following a fastboot), we need to do a full mode set. v2: - put under pipe_config check so we don't deref a null state (Jesse) Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)