diff mbox series

[3/4] drm/panel: sitronix-st7789v: add support for partial mode

Message ID 20230718-feature-lcd-panel-v1-3-e9a85d5374fd@wolfvision.net
State New
Headers
Series drm/panel: sitronix-st7789v: add support for partial mode |

Commit Message

Michael Riesch July 18, 2023, 3:31 p.m. UTC 
  The ST7789V controller features support for the partial mode. Here,
the area to be displayed can be restricted in one direction (by default,
in vertical direction). This is useful for panels that are partially
occluded by design. Add support for the partial mode.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 38 ++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

Comments

Maxime Ripard July 19, 2023, 6:39 a.m. UTC | #1 
Hi,

On Tue, Jul 18, 2023 at 05:31:52PM +0200, Michael Riesch wrote:
> The ST7789V controller features support for the partial mode. Here,
> the area to be displayed can be restricted in one direction (by default,
> in vertical direction). This is useful for panels that are partially
> occluded by design. Add support for the partial mode.
> 
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>

We already had that discussion, but I think we shouldn't treat this any
differently than overscan for other output.

Maxime
Michael Riesch Aug. 2, 2023, 12:34 p.m. UTC | #2 
Hi Maxime,

On 7/19/23 08:39, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Jul 18, 2023 at 05:31:52PM +0200, Michael Riesch wrote:
>> The ST7789V controller features support for the partial mode. Here,
>> the area to be displayed can be restricted in one direction (by default,
>> in vertical direction). This is useful for panels that are partially
>> occluded by design. Add support for the partial mode.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> 
> We already had that discussion, but I think we shouldn't treat this any
> differently than overscan for other output.

Indeed we had that discussion. For reference, it can be found here:
https://lore.kernel.org/dri-devel/20230329091636.mu6ml3gvw5mvkhm4@penduick/#t
The thing is that I am still clueless how the overscan approach could work.

I found some DRM properties related to overscan/margins and I guess
userspace needs to set those. On my system weston is running. Is weston
in charge of configuring the corresponding output so that the correct
margins are applied? If so, how can this be achieved?

Will DRM handle the properties generically or does the driver need to do
some work as well?

In any case it could make sense to write the partial mode registers and
enter the effective dimensions. At least I have seen this in other panel
drivers.

Thanks and best regards,
Michael

> 
> Maxime
Maxime Ripard Aug. 2, 2023, 12:47 p.m. UTC | #3 
Hi,

On Wed, Aug 02, 2023 at 02:34:28PM +0200, Michael Riesch wrote:
> On 7/19/23 08:39, Maxime Ripard wrote:
> > On Tue, Jul 18, 2023 at 05:31:52PM +0200, Michael Riesch wrote:
> >> The ST7789V controller features support for the partial mode. Here,
> >> the area to be displayed can be restricted in one direction (by default,
> >> in vertical direction). This is useful for panels that are partially
> >> occluded by design. Add support for the partial mode.
> >>
> >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> > 
> > We already had that discussion, but I think we shouldn't treat this any
> > differently than overscan for other output.
> 
> Indeed we had that discussion. For reference, it can be found here:
> https://lore.kernel.org/dri-devel/20230329091636.mu6ml3gvw5mvkhm4@penduick/#t
> The thing is that I am still clueless how the overscan approach could work.
> 
> I found some DRM properties related to overscan/margins and I guess
> userspace needs to set those. On my system weston is running. Is weston
> in charge of configuring the corresponding output so that the correct
> margins are applied? If so, how can this be achieved?

I don't really know Weston, but my guess would be based on some
configuration or user feedback, depending on which case we're in.

We also set the default using some kernel command-line options.

> Will DRM handle the properties generically or does the driver need to do
> some work as well?

What do you mean by generically?

> In any case it could make sense to write the partial mode registers and
> enter the effective dimensions. At least I have seen this in other panel
> drivers.

Sure, it makes sense. It shouldn't come from the DT and be fixed though.

Maxime
Michael Riesch Aug. 2, 2023, 3:03 p.m. UTC | #4 
Hi all,

In order to avoid spamming the list, I sparked a discussion in
#dri-devel. FTR the log can be found here:
https://oftc.irclog.whitequark.org/dri-devel/2023-08-02#32360491;

On 8/2/23 14:47, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Aug 02, 2023 at 02:34:28PM +0200, Michael Riesch wrote:
>> On 7/19/23 08:39, Maxime Ripard wrote:
>>> On Tue, Jul 18, 2023 at 05:31:52PM +0200, Michael Riesch wrote:
>>>> The ST7789V controller features support for the partial mode. Here,
>>>> the area to be displayed can be restricted in one direction (by default,
>>>> in vertical direction). This is useful for panels that are partially
>>>> occluded by design. Add support for the partial mode.
>>>>
>>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>>>
>>> We already had that discussion, but I think we shouldn't treat this any
>>> differently than overscan for other output.
>>
>> Indeed we had that discussion. For reference, it can be found here:
>> https://lore.kernel.org/dri-devel/20230329091636.mu6ml3gvw5mvkhm4@penduick/#t
>> The thing is that I am still clueless how the overscan approach could work.
>>
>> I found some DRM properties related to overscan/margins and I guess
>> userspace needs to set those. On my system weston is running. Is weston
>> in charge of configuring the corresponding output so that the correct
>> margins are applied? If so, how can this be achieved?
> 
> I don't really know Weston, but my guess would be based on some
> configuration or user feedback, depending on which case we're in.
> 
> We also set the default using some kernel command-line options.
> 
>> Will DRM handle the properties generically or does the driver need to do
>> some work as well?
> 
> What do you mean by generically?

I guess my question can be reduced to "What does the driver have to do
to support this overscan thingy?" If the overscan approach is the
preferred one, then I'd appreciate some pointers as to how this could work.

>> In any case it could make sense to write the partial mode registers and
>> enter the effective dimensions. At least I have seen this in other panel
>> drivers.
> 
> Sure, it makes sense. It shouldn't come from the DT and be fixed though.

However, as indicated in Daniel Vetter's summary of the IRC discussion,
the overscan properties may not be the preferred solution in this case.

Looking forward to further comments (alternatively, to seeing this patch
series getting applied :-))

Best regards,
Michael
Neil Armstrong Aug. 4, 2023, 8:41 a.m. UTC | #5 
Hi Michael,

On 18/07/2023 17:31, Michael Riesch wrote:
> The ST7789V controller features support for the partial mode. Here,
> the area to be displayed can be restricted in one direction (by default,
> in vertical direction). This is useful for panels that are partial > occluded by design. Add support for the partial mode.

Could you send a v2 with a comment in the code as Maxime suggests ?

Thanks,
Neil

> 
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
>   drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 38 ++++++++++++++++++++++++--
>   1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index d16d17f21d92..729d8d7dbf7f 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -118,6 +118,9 @@ struct st7789_panel_info {
>   	u32 bus_format;
>   	u32 bus_flags;
>   	bool invert_mode;
> +	bool partial_mode;
> +	u16 partial_start;
> +	u16 partial_end;
>   };
>   
>   struct st7789v {
> @@ -330,9 +333,14 @@ static int st7789v_get_modes(struct drm_panel *panel,
>   static int st7789v_prepare(struct drm_panel *panel)
>   {
>   	struct st7789v *ctx = panel_to_st7789v(panel);
> -	u8 pixel_fmt, polarity;
> +	u8 mode, pixel_fmt, polarity;
>   	int ret;
>   
> +	if (!ctx->info->partial_mode)
> +		mode = ST7789V_RGBCTRL_WO;
> +	else
> +		mode = 0;
> +
>   	switch (ctx->info->bus_format) {
>   	case MEDIA_BUS_FMT_RGB666_1X18:
>   		pixel_fmt = MIPI_DCS_PIXEL_FMT_18BIT;
> @@ -472,6 +480,32 @@ static int st7789v_prepare(struct drm_panel *panel)
>   						MIPI_DCS_EXIT_INVERT_MODE));
>   	}
>   
> +	if (ctx->info->partial_mode) {
> +		u8 area_data[4] = {
> +			(ctx->info->partial_start >> 8) & 0xff,
> +			(ctx->info->partial_start >> 0) & 0xff,
> +			((ctx->info->partial_end - 1) >> 8) & 0xff,
> +			((ctx->info->partial_end - 1) >> 0) & 0xff,
> +		};
> +
> +		ST7789V_TEST(ret, st7789v_write_command(
> +					  ctx, MIPI_DCS_ENTER_PARTIAL_MODE));
> +
> +		ST7789V_TEST(ret, st7789v_write_command(
> +					  ctx, MIPI_DCS_SET_PAGE_ADDRESS));
> +		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
> +		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
> +		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
> +		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
> +
> +		ST7789V_TEST(ret, st7789v_write_command(
> +					  ctx, MIPI_DCS_SET_PARTIAL_ROWS));
> +		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
> +		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
> +		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
> +		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
> +	}
> +
>   	ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RAMCTRL_CMD));
>   	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RAMCTRL_DM_RGB |
>   					     ST7789V_RAMCTRL_RM_RGB));
> @@ -479,7 +513,7 @@ static int st7789v_prepare(struct drm_panel *panel)
>   					     ST7789V_RAMCTRL_MAGIC));
>   
>   	ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RGBCTRL_CMD));
> -	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_WO |
> +	ST7789V_TEST(ret, st7789v_write_data(ctx, mode |
>   					     ST7789V_RGBCTRL_RCM(2) |
>   					     polarity));
>   	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
>
Michael Riesch Aug. 4, 2023, 9:45 a.m. UTC | #6 
Hi Neil,

On 8/4/23 10:41, Neil Armstrong wrote:
> Hi Michael,
> 
> On 18/07/2023 17:31, Michael Riesch wrote:
>> The ST7789V controller features support for the partial mode. Here,
>> the area to be displayed can be restricted in one direction (by default,
>> in vertical direction). This is useful for panels that are partial >
>> occluded by design. Add support for the partial mode.
> 
> Could you send a v2 with a comment in the code as Maxime suggests ?

Sure thing! I must admit that I do not understand his concerns exactly,
though.

@Maxime: I can prepare a suggestion but feel free to tell me the exact
wording at the preferred position.

Best regards,
Michael

> 
> Thanks,
> Neil
> 
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>>   drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 38
>> ++++++++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> index d16d17f21d92..729d8d7dbf7f 100644
>> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> @@ -118,6 +118,9 @@ struct st7789_panel_info {
>>       u32 bus_format;
>>       u32 bus_flags;
>>       bool invert_mode;
>> +    bool partial_mode;
>> +    u16 partial_start;
>> +    u16 partial_end;
>>   };
>>     struct st7789v {
>> @@ -330,9 +333,14 @@ static int st7789v_get_modes(struct drm_panel
>> *panel,
>>   static int st7789v_prepare(struct drm_panel *panel)
>>   {
>>       struct st7789v *ctx = panel_to_st7789v(panel);
>> -    u8 pixel_fmt, polarity;
>> +    u8 mode, pixel_fmt, polarity;
>>       int ret;
>>   +    if (!ctx->info->partial_mode)
>> +        mode = ST7789V_RGBCTRL_WO;
>> +    else
>> +        mode = 0;
>> +
>>       switch (ctx->info->bus_format) {
>>       case MEDIA_BUS_FMT_RGB666_1X18:
>>           pixel_fmt = MIPI_DCS_PIXEL_FMT_18BIT;
>> @@ -472,6 +480,32 @@ static int st7789v_prepare(struct drm_panel *panel)
>>                           MIPI_DCS_EXIT_INVERT_MODE));
>>       }
>>   +    if (ctx->info->partial_mode) {
>> +        u8 area_data[4] = {
>> +            (ctx->info->partial_start >> 8) & 0xff,
>> +            (ctx->info->partial_start >> 0) & 0xff,
>> +            ((ctx->info->partial_end - 1) >> 8) & 0xff,
>> +            ((ctx->info->partial_end - 1) >> 0) & 0xff,
>> +        };
>> +
>> +        ST7789V_TEST(ret, st7789v_write_command(
>> +                      ctx, MIPI_DCS_ENTER_PARTIAL_MODE));
>> +
>> +        ST7789V_TEST(ret, st7789v_write_command(
>> +                      ctx, MIPI_DCS_SET_PAGE_ADDRESS));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
>> +
>> +        ST7789V_TEST(ret, st7789v_write_command(
>> +                      ctx, MIPI_DCS_SET_PARTIAL_ROWS));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
>> +    }
>> +
>>       ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RAMCTRL_CMD));
>>       ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RAMCTRL_DM_RGB |
>>                            ST7789V_RAMCTRL_RM_RGB));
>> @@ -479,7 +513,7 @@ static int st7789v_prepare(struct drm_panel *panel)
>>                            ST7789V_RAMCTRL_MAGIC));
>>         ST7789V_TEST(ret, st7789v_write_command(ctx,
>> ST7789V_RGBCTRL_CMD));
>> -    ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_WO |
>> +    ST7789V_TEST(ret, st7789v_write_data(ctx, mode |
>>                            ST7789V_RGBCTRL_RCM(2) |
>>                            polarity));
>>       ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
>>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index d16d17f21d92..729d8d7dbf7f 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -118,6 +118,9 @@  struct st7789_panel_info {
 	u32 bus_format;
 	u32 bus_flags;
 	bool invert_mode;
+	bool partial_mode;
+	u16 partial_start;
+	u16 partial_end;
 };
 
 struct st7789v {
@@ -330,9 +333,14 @@  static int st7789v_get_modes(struct drm_panel *panel,
 static int st7789v_prepare(struct drm_panel *panel)
 {
 	struct st7789v *ctx = panel_to_st7789v(panel);
-	u8 pixel_fmt, polarity;
+	u8 mode, pixel_fmt, polarity;
 	int ret;
 
+	if (!ctx->info->partial_mode)
+		mode = ST7789V_RGBCTRL_WO;
+	else
+		mode = 0;
+
 	switch (ctx->info->bus_format) {
 	case MEDIA_BUS_FMT_RGB666_1X18:
 		pixel_fmt = MIPI_DCS_PIXEL_FMT_18BIT;
@@ -472,6 +480,32 @@  static int st7789v_prepare(struct drm_panel *panel)
 						MIPI_DCS_EXIT_INVERT_MODE));
 	}
 
+	if (ctx->info->partial_mode) {
+		u8 area_data[4] = {
+			(ctx->info->partial_start >> 8) & 0xff,
+			(ctx->info->partial_start >> 0) & 0xff,
+			((ctx->info->partial_end - 1) >> 8) & 0xff,
+			((ctx->info->partial_end - 1) >> 0) & 0xff,
+		};
+
+		ST7789V_TEST(ret, st7789v_write_command(
+					  ctx, MIPI_DCS_ENTER_PARTIAL_MODE));
+
+		ST7789V_TEST(ret, st7789v_write_command(
+					  ctx, MIPI_DCS_SET_PAGE_ADDRESS));
+		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
+		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
+		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
+		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
+
+		ST7789V_TEST(ret, st7789v_write_command(
+					  ctx, MIPI_DCS_SET_PARTIAL_ROWS));
+		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
+		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
+		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
+		ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
+	}
+
 	ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RAMCTRL_CMD));
 	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RAMCTRL_DM_RGB |
 					     ST7789V_RAMCTRL_RM_RGB));
@@ -479,7 +513,7 @@  static int st7789v_prepare(struct drm_panel *panel)
 					     ST7789V_RAMCTRL_MAGIC));
 
 	ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RGBCTRL_CMD));
-	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_WO |
+	ST7789V_TEST(ret, st7789v_write_data(ctx, mode |
 					     ST7789V_RGBCTRL_RCM(2) |
 					     polarity));
 	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));