| Message ID | 20161201202925.12220-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 01, 2016 at 09:29:08PM +0100, Hans de Goede wrote: > Looking at the ADF code from the Android kernel sources for a > cherrytrail tablet I noticed that it is calling the > MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook. > > Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences > in panel prepare/unprepare hooks") the mainline i915 code was doing the > same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET / > MIPI_SEQ_DEASSERT_RESET. > > Looking at the naming of the sequences that is the right thing to do, > but the problem is, that the old mainline code and the ADF code was > actually calling the right sequence (tested on a cube iwork8 air tablet), > and the swapping of the calling breaks things. > > This breakage was likely not noticed in testing because on cherrytrail, > currently chv_exec_gpio ends up disabling the gpio pins rather then > setting them (this is fixed in the next patch in this patch-set). > > This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's > places in the enum defining them, so that their (new) names match their > actual use. > > Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...") > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/i915/intel_bios.h | 4 ++-- > drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h > index 8405b5a..642a5eb 100644 > --- a/drivers/gpu/drm/i915/intel_bios.h > +++ b/drivers/gpu/drm/i915/intel_bios.h > @@ -49,11 +49,11 @@ struct edp_power_seq { > /* MIPI Sequence Block definitions */ > enum mipi_seq { > MIPI_SEQ_END = 0, > - MIPI_SEQ_ASSERT_RESET, > + MIPI_SEQ_DEASSERT_RESET, > MIPI_SEQ_INIT_OTP, > MIPI_SEQ_DISPLAY_ON, > MIPI_SEQ_DISPLAY_OFF, > - MIPI_SEQ_DEASSERT_RESET, > + MIPI_SEQ_ASSERT_RESET, I think we'll still want to keep to the names as they are in the spec, and instead we'll just call them in the order that looks wrong + add a comment explaining why we do that. > MIPI_SEQ_BACKLIGHT_ON, /* sequence block v2+ */ > MIPI_SEQ_BACKLIGHT_OFF, /* sequence block v2+ */ > MIPI_SEQ_TEAR_ON, /* sequence block v2+ */ > diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > index 0d8ff00..579d2f5 100644 > --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = { > */ > > static const char * const seq_name[] = { > - [MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET", > + [MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET", > [MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP", > [MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON", > [MIPI_SEQ_DISPLAY_OFF] = "MIPI_SEQ_DISPLAY_OFF", > - [MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET", > + [MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET", > [MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON", > [MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF", > [MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON", > -- > 2.9.3
Hi, On 02-12-16 13:37, Ville Syrjälä wrote: > On Thu, Dec 01, 2016 at 09:29:08PM +0100, Hans de Goede wrote: >> Looking at the ADF code from the Android kernel sources for a >> cherrytrail tablet I noticed that it is calling the >> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook. >> >> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences >> in panel prepare/unprepare hooks") the mainline i915 code was doing the >> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET / >> MIPI_SEQ_DEASSERT_RESET. >> >> Looking at the naming of the sequences that is the right thing to do, >> but the problem is, that the old mainline code and the ADF code was >> actually calling the right sequence (tested on a cube iwork8 air tablet), >> and the swapping of the calling breaks things. >> >> This breakage was likely not noticed in testing because on cherrytrail, >> currently chv_exec_gpio ends up disabling the gpio pins rather then >> setting them (this is fixed in the next patch in this patch-set). >> >> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's >> places in the enum defining them, so that their (new) names match their >> actual use. >> >> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...") >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/i915/intel_bios.h | 4 ++-- >> drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++-- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h >> index 8405b5a..642a5eb 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.h >> +++ b/drivers/gpu/drm/i915/intel_bios.h >> @@ -49,11 +49,11 @@ struct edp_power_seq { >> /* MIPI Sequence Block definitions */ >> enum mipi_seq { >> MIPI_SEQ_END = 0, >> - MIPI_SEQ_ASSERT_RESET, >> + MIPI_SEQ_DEASSERT_RESET, >> MIPI_SEQ_INIT_OTP, >> MIPI_SEQ_DISPLAY_ON, >> MIPI_SEQ_DISPLAY_OFF, >> - MIPI_SEQ_DEASSERT_RESET, >> + MIPI_SEQ_ASSERT_RESET, > > I think we'll still want to keep to the names as they are in the > spec, and instead we'll just call them in the order that looks > wrong + add a comment explaining why we do that. I really want the i915 driver to use the correct names, anything else will lead to no amount of confusion for people who have experience with embedded stuff. How about adding a comment here in the enum, as well as in patch 8: "drm/i915/dsi: Document the panel enable / disable sequences from the spec" That the spec has assert / deassert the wrong way around and that the i915 code is using the correct names ? Most people will not even have access to the spec, so it seems to me that having this right in the code, with a comment to warn people who do have access to the spec is better then the other way around. Regards, Hans > >> MIPI_SEQ_BACKLIGHT_ON, /* sequence block v2+ */ >> MIPI_SEQ_BACKLIGHT_OFF, /* sequence block v2+ */ >> MIPI_SEQ_TEAR_ON, /* sequence block v2+ */ >> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >> index 0d8ff00..579d2f5 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c >> @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = { >> */ >> >> static const char * const seq_name[] = { >> - [MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET", >> + [MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET", >> [MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP", >> [MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON", >> [MIPI_SEQ_DISPLAY_OFF] = "MIPI_SEQ_DISPLAY_OFF", >> - [MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET", >> + [MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET", >> [MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON", >> [MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF", >> [MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON", >> -- >> 2.9.3 >
On Fri, Dec 02, 2016 at 02:34:01PM +0100, Hans de Goede wrote: > Hi, > > On 02-12-16 13:37, Ville Syrjälä wrote: > > On Thu, Dec 01, 2016 at 09:29:08PM +0100, Hans de Goede wrote: > >> Looking at the ADF code from the Android kernel sources for a > >> cherrytrail tablet I noticed that it is calling the > >> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook. > >> > >> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences > >> in panel prepare/unprepare hooks") the mainline i915 code was doing the > >> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET / > >> MIPI_SEQ_DEASSERT_RESET. > >> > >> Looking at the naming of the sequences that is the right thing to do, > >> but the problem is, that the old mainline code and the ADF code was > >> actually calling the right sequence (tested on a cube iwork8 air tablet), > >> and the swapping of the calling breaks things. > >> > >> This breakage was likely not noticed in testing because on cherrytrail, > >> currently chv_exec_gpio ends up disabling the gpio pins rather then > >> setting them (this is fixed in the next patch in this patch-set). > >> > >> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's > >> places in the enum defining them, so that their (new) names match their > >> actual use. > >> > >> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...") > >> Cc: Jani Nikula <jani.nikula@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/gpu/drm/i915/intel_bios.h | 4 ++-- > >> drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++-- > >> 2 files changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h > >> index 8405b5a..642a5eb 100644 > >> --- a/drivers/gpu/drm/i915/intel_bios.h > >> +++ b/drivers/gpu/drm/i915/intel_bios.h > >> @@ -49,11 +49,11 @@ struct edp_power_seq { > >> /* MIPI Sequence Block definitions */ > >> enum mipi_seq { > >> MIPI_SEQ_END = 0, > >> - MIPI_SEQ_ASSERT_RESET, > >> + MIPI_SEQ_DEASSERT_RESET, > >> MIPI_SEQ_INIT_OTP, > >> MIPI_SEQ_DISPLAY_ON, > >> MIPI_SEQ_DISPLAY_OFF, > >> - MIPI_SEQ_DEASSERT_RESET, > >> + MIPI_SEQ_ASSERT_RESET, > > > > I think we'll still want to keep to the names as they are in the > > spec, and instead we'll just call them in the order that looks > > wrong + add a comment explaining why we do that. > > I really want the i915 driver to use the correct names, anything > else will lead to no amount of confusion for people who have > experience with embedded stuff. > > How about adding a comment here in the enum, as well as in > patch 8: "drm/i915/dsi: Document the panel enable / disable sequences from the spec" > That the spec has assert / deassert the wrong way around and that > the i915 code is using the correct names ? Sure, I could live with that. We definitely need to have that comment somewhere. > > Most people will not even have access to the spec, so it seems to > me that having this right in the code, with a comment to warn > people who do have access to the spec is better then the other > way around. > > Regards, > > Hans > > > > > > >> MIPI_SEQ_BACKLIGHT_ON, /* sequence block v2+ */ > >> MIPI_SEQ_BACKLIGHT_OFF, /* sequence block v2+ */ > >> MIPI_SEQ_TEAR_ON, /* sequence block v2+ */ > >> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > >> index 0d8ff00..579d2f5 100644 > >> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > >> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > >> @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = { > >> */ > >> > >> static const char * const seq_name[] = { > >> - [MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET", > >> + [MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET", > >> [MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP", > >> [MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON", > >> [MIPI_SEQ_DISPLAY_OFF] = "MIPI_SEQ_DISPLAY_OFF", > >> - [MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET", > >> + [MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET", > >> [MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON", > >> [MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF", > >> [MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON", > >> -- > >> 2.9.3 > >
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h index 8405b5a..642a5eb 100644 --- a/drivers/gpu/drm/i915/intel_bios.h +++ b/drivers/gpu/drm/i915/intel_bios.h @@ -49,11 +49,11 @@ struct edp_power_seq { /* MIPI Sequence Block definitions */ enum mipi_seq { MIPI_SEQ_END = 0, - MIPI_SEQ_ASSERT_RESET, + MIPI_SEQ_DEASSERT_RESET, MIPI_SEQ_INIT_OTP, MIPI_SEQ_DISPLAY_ON, MIPI_SEQ_DISPLAY_OFF, - MIPI_SEQ_DEASSERT_RESET, + MIPI_SEQ_ASSERT_RESET, MIPI_SEQ_BACKLIGHT_ON, /* sequence block v2+ */ MIPI_SEQ_BACKLIGHT_OFF, /* sequence block v2+ */ MIPI_SEQ_TEAR_ON, /* sequence block v2+ */ diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c index 0d8ff00..579d2f5 100644 --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = { */ static const char * const seq_name[] = { - [MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET", + [MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET", [MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP", [MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON", [MIPI_SEQ_DISPLAY_OFF] = "MIPI_SEQ_DISPLAY_OFF", - [MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET", + [MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET", [MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON", [MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF", [MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
Looking at the ADF code from the Android kernel sources for a cherrytrail tablet I noticed that it is calling the MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook. Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences in panel prepare/unprepare hooks") the mainline i915 code was doing the same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET / MIPI_SEQ_DEASSERT_RESET. Looking at the naming of the sequences that is the right thing to do, but the problem is, that the old mainline code and the ADF code was actually calling the right sequence (tested on a cube iwork8 air tablet), and the swapping of the calling breaks things. This breakage was likely not noticed in testing because on cherrytrail, currently chv_exec_gpio ends up disabling the gpio pins rather then setting them (this is fixed in the next patch in this patch-set). This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's places in the enum defining them, so that their (new) names match their actual use. Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...") Cc: Jani Nikula <jani.nikula@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/intel_bios.h | 4 ++-- drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)